3 ms·

1. naming conventions are unusual and inconsistent (List_destroy but then there's destroy_svcs_list) 2. the return value of malloc() et al. isn't checked (htt

by cremno 11y ago

1. naming conventions are unusual and inconsistent (List_destroy but then there's destroy_svcs_list) 2. the return value of malloc() et al. isn't checked (http://www.etalabs.net/overcommit.html http://www.etalabs.net/overcommit.html) 3. the register storage-class specifier is used multiple times (mainly lib/s16db/translate.c) 4. function declarations are missing a prototype (() instead of (void))

nkurz 11y ago
I'm not claiming the code is perfect. I'm claiming that at a glance it looks like professional code of a quality similar that of other high quality C codebases: Linux, Python, Perl, Apache, sqlite, etc. One can always do better. I (and likely the author) disagree with you on the importance of checking the return value of malloc(). Because of overcommit (as you point out), a non-null value returned from malloc() does not mean that you will not crash when you access the pointer. If the pointer is used directly, crash on NULL might a reasonable approach. It's when NULL is retained and then used as the base of an array that it may become a security problem. Configuring malloc() to abort() on failure would be probably my preferred solution. I agree with the last point, but think it's a minor one. While I'd like C to treat () in a function definition as equivalent to (void), for historical reasons it does not. The author trades off the visual noise of the word 'void' for better error reporting. But depending on your compiler, you may still get a warning. On the computer I just tried, 'icc' and 'clang' gave clear warnings but 'gcc' did not. nate@ubuntu:~/C$ cat void.c #include <stdio.h> int empty() { return 3; } int main(void) { printf("%d\n", empty(4)); return 0; } nate@ubuntu:~/C$ icc -Wall -o void void.c void.c(3): warning #140: too many arguments in function call int main(void) { printf("%d\n", empty(4)); return 0; } ^ nate@ubuntu:~/C$ clang -Wall -o void void.c void.c:3:40: warning: too many arguments in call to 'empty' int main(void) { printf("%d\n", empty(4)); return 0; } ~~~~~ ^
- AndyKelley 11y ago
  Not all systems have overcommit enabled. Notably, as the article linked above points out, robust systems do not have overcommit enabled. The OOM killer is heuristics based and cannot be relied on in robust systems. This means that libraries and applications that want to be viable on such platforms have to recognize that malloc may return NULL and respond accordingly.
  - EmanueleAina 11y ago
    Very true for low-level system libraries, but applications should really not waste energy on trying to be OOM-safe: it's hard to understand which actions are safe when you no longer can allocate memory (releasing resources often requires allocating memory temporarily), and it just adds complexity which basically will never get tested, thus getting broken quickly and gaining nothing over just crashing on a NULL dereference. See the experience of the D-Bus (which tries hard to be OOM-safe) on this topic: http://blog.ometer.com/2008/02/04/out-of-memory-handling-d-bus-experience/ http://blog.ometer.com/2008/02/04/out-of-memory-handling-d-b...

by cremno 11y ago

nkurz 11y ago

I'm not claiming the code is perfect. I'm claiming that at a glance it looks like professional code of a quality similar that of other high quality C codebases: Linux, Python, Perl, Apache, sqlite, etc. One can always do better. I (and likely the author) disagree with you on the importance of checking the return value of malloc(). Because of overcommit (as you point out), a non-null value returned from malloc() does not mean that you will not crash when you access the pointer. If the pointer is used directly, crash on NULL might a reasonable approach. It's when NULL is retained and then used as the base of an array that it may become a security problem. Configuring malloc() to abort() on failure would be probably my preferred solution. I agree with the last point, but think it's a minor one. While I'd like C to treat () in a function definition as equivalent to (void), for historical reasons it does not. The author trades off the visual noise of the word 'void' for better error reporting. But depending on your compiler, you may still get a warning. On the computer I just tried, 'icc' and 'clang' gave clear warnings but 'gcc' did not. nate@ubuntu:~/C$ cat void.c #include <stdio.h> int empty() { return 3; } int main(void) { printf("%d\n", empty(4)); return 0; } nate@ubuntu:~/C$ icc -Wall -o void void.c void.c(3): warning #140: too many arguments in function call int main(void) { printf("%d\n", empty(4)); return 0; } ^ nate@ubuntu:~/C$ clang -Wall -o void void.c void.c:3:40: warning: too many arguments in call to 'empty' int main(void) { printf("%d\n", empty(4)); return 0; } ~~~~~ ^

AndyKelley 11y ago
Not all systems have overcommit enabled. Notably, as the article linked above points out, robust systems do not have overcommit enabled. The OOM killer is heuristics based and cannot be relied on in robust systems. This means that libraries and applications that want to be viable on such platforms have to recognize that malloc may return NULL and respond accordingly.
- EmanueleAina 11y ago
  Very true for low-level system libraries, but applications should really not waste energy on trying to be OOM-safe: it's hard to understand which actions are safe when you no longer can allocate memory (releasing resources often requires allocating memory temporarily), and it just adds complexity which basically will never get tested, thus getting broken quickly and gaining nothing over just crashing on a NULL dereference. See the experience of the D-Bus (which tries hard to be OOM-safe) on this topic: http://blog.ometer.com/2008/02/04/out-of-memory-handling-d-bus-experience/ http://blog.ometer.com/2008/02/04/out-of-memory-handling-d-b...