4 ms·

Few nitpicks found while skimming through the source. fd = socket(AF_INET6, SOCK_DGRAM, 0); if (fd <= 0) { This should be fd == -1. fd == 0 is valid. htt

by halayli 12y ago

Few nitpicks found while skimming through the source. fd = socket(AF_INET6, SOCK_DGRAM, 0); if (fd <= 0) { This should be fd == -1. fd == 0 is valid. https://github.com/robertdavidgraham/robdns/blob/master/src/main-server-socket.c#L167 https://github.com/robertdavidgraham/robdns/blob/master/src/... https://github.com/robertdavidgraham/robdns/blob/master/src/main-server-socket.c#L126 https://github.com/robertdavidgraham/robdns/blob/master/src/... check errors from sendto() & recvfrom(). you don't want to loop infinitely when fd is hosed. https://github.com/robertdavidgraham/robdns/blob/master/src/grind.c#L43 https://github.com/robertdavidgraham/robdns/blob/master/src/... Check return value of your mallocs. https://github.com/robertdavidgraham/robdns/blob/master/src/pixie-nic.c#L328 https://github.com/robertdavidgraham/robdns/blob/master/src/... Check return value of fd.

tptacek 12y ago
The zero fd is valid but will never happen in this code. Checking the return value of malloc is a bad idea (just rig the program to explode when any malloc fails). But casting the return value of malloc is incorrect, and that code should be using calloc.
- stonogo 12y ago
  > Checking the return value of malloc is a bad idea I am embarassed for you.
  - tptacek 12y ago
    That's fascinating. Tell me more.
    - dkhenry 12y ago
      He clearly has never heard of mallopt (http://man7.org/linux/man-pages/man3/mallopt.3.html http://man7.org/linux/man-pages/man3/mallopt.3.html) so he doesn't know that instead of wrapping every single malloc with an if statement you can just tell malloc to abrt if it ever fails to allocate memory. In his defense most programs don't test the return of malloc and don't set the abort on failure so most programmers would just assume that if your not checking your return values that you made a mistake.
    - stonogo 12y ago
      If a daemon is not capable of surviving memory exhaustion, it is not suitable for deployment. If a programmer is not capable of writing software that can survive memory exhaustion, they are not suitable for writing daemons. I'll not argue the point further; the HN lynchmob is already beating at my door for daring to disagree with a site darling. Had I looked at your username before replying, I probably would have forborn from violating the bubble.
      - SamReidHughes 12y ago
        Quoting and with total vacuity, replying, "I am embarassed for you," will get you downvoted no matter who the parent's author is.
      - tptacek 12y ago
        What's a single example of a daemon with more than 10,000 lines of C code that will reliably "survive" memory exhaustion? By "survive" we obviously both mean that it retains its original PID. Of course, most Unix serverside code "survives" by simply aborting and allowing itself to be restarted. Since that's what I'm recommending, that strategy doesn't count. To find one, you're going to have to catalog every malloc() in the entire program and record some kind of recovery regime --- maybe degraded performance, maybe dropped requests, maybe fallback to some kind of pool --- for every allocation.
        halayli 12y ago
        An in-memory message queue server or memcached variations don't need to crash if it can't serve a particular request that requires more memory than available. I am not sure why error-handling is a difficult concept for you.
        tptacek 12y ago
        You just moved the goalposts. Yes, it's suboptimal to have a server that receives a request with a 32 bit length field indicating 3 gigs of incoming data that the bombs out trying to malloc 32 gigs. I'm saying: what's an example of a server that reliably, for every allocation of every piece of metadata, every strdup, every hash table entry, every connection object, &c &c, has a recovery regime so that it doesn't have to fail ever when malloc does? One way you could find such a program would be to compile a candidate and preload a malloc that randomly (1 in every 100 calls per allocation size, for instance) returns NULL. See if the program (a) continues to run and (b) passes some simple unit test suite. My contention is: (a) Those programs will be hard to find, because most "production ready" Unix code does not have that property, and (b) The criteria I'm talking about matters a lot, because (1) memory exhaustion strikes at totally arbitrary points in a program's execution, not just at the points where you're prepared to handle it, and (2) attackers can pinpoint exactly the allocation they want to have fail. As an example of an approach that doesn't seem to work well: in "memqueue", the function that creates HTTP headers in responses allocates an array of iovecs. If that malloc fails, the header creation function returns -1. The function that calls the header-creation function, http_respond, catches that error and itself returns -1. Nothing ever checks the error return from http_respond. (Also, the loop in which memqueue reads entire requests into memory by continuously realloc()'ing a receive buffer has an integer wrap bug in it, though it's probably not triggerable. But incorrect is incorrect, right?)
- halayli 12y ago
  How do you know? What if someone forked this program and closed fd 0 before execX()? You have to abide by the posix standard and what the man page tells you. Now it never happens, but later the code can change and the case can hit. Most probably you'd have forgotten about this check. Malloc failures don't always have to result in aborting the program. Cases can vary. My suggestion to you is abide by the man page and always check error conditions. Don't overthink the failure cases of stdlib & posix calls. You never know the OS/environment your program will run in, and the only common ground you have is the standard.
  - tptacek 12y ago
    You don't know. It's true that the line of code he cited was incorrect. It's just not a very interesting example of incorrect code. Malloc failures don't always result in aborting. The common alternative, especially in programs that have careful malloc return value checking regimes, is to occasionally cough up remote code execution. Userland systems programmers should assume the conservative default of ending the program immediately when malloc fails. A similar logic guided C++ into throwing bad_alloc instead of returning NULL on allocation failures. And a survey of modern C++ code will show you that most C++ programs simply allow themselves to terminate when bad_alloc happens.
    - halayli 12y ago
      It doesn't have to be interesting. Incorrect code covers a spectrum and is not black or white. The term "edge-case" describes it pretty well. Regarding mallocs, again you don't have to overthink error-handling. Just handle it and figure out how to deal with it if it happens. C++ has nothrow and try/catch. If you want to catch a failed new or abort is something up to the user. One thing for sure is that C++ aborts on failed new instead of stumping on unallocated memory. On the other hand, consistency is important. Some parts of the code handled fd correctly and malloc failures and other parts did not. I recommend you applaud good programming practices and criticize bad ones. Defending/arguing bad programming practices(whether they are edge cases or not) doesn't help.
      - 12y ago

by halayli 12y ago

tptacek 12y ago

The zero fd is valid but will never happen in this code. Checking the return value of malloc is a bad idea (just rig the program to explode when any malloc fails). But casting the return value of malloc is incorrect, and that code should be using calloc.

stonogo 12y ago
> Checking the return value of malloc is a bad idea I am embarassed for you.
- tptacek 12y ago
  That's fascinating. Tell me more.
  - dkhenry 12y ago
    He clearly has never heard of mallopt (http://man7.org/linux/man-pages/man3/mallopt.3.html http://man7.org/linux/man-pages/man3/mallopt.3.html) so he doesn't know that instead of wrapping every single malloc with an if statement you can just tell malloc to abrt if it ever fails to allocate memory. In his defense most programs don't test the return of malloc and don't set the abort on failure so most programmers would just assume that if your not checking your return values that you made a mistake.
  - stonogo 12y ago
    If a daemon is not capable of surviving memory exhaustion, it is not suitable for deployment. If a programmer is not capable of writing software that can survive memory exhaustion, they are not suitable for writing daemons. I'll not argue the point further; the HN lynchmob is already beating at my door for daring to disagree with a site darling. Had I looked at your username before replying, I probably would have forborn from violating the bubble.
    - SamReidHughes 12y ago
      Quoting and with total vacuity, replying, "I am embarassed for you," will get you downvoted no matter who the parent's author is.
    - tptacek 12y ago
      What's a single example of a daemon with more than 10,000 lines of C code that will reliably "survive" memory exhaustion? By "survive" we obviously both mean that it retains its original PID. Of course, most Unix serverside code "survives" by simply aborting and allowing itself to be restarted. Since that's what I'm recommending, that strategy doesn't count. To find one, you're going to have to catalog every malloc() in the entire program and record some kind of recovery regime --- maybe degraded performance, maybe dropped requests, maybe fallback to some kind of pool --- for every allocation.
      - halayli 12y ago
        An in-memory message queue server or memcached variations don't need to crash if it can't serve a particular request that requires more memory than available. I am not sure why error-handling is a difficult concept for you.
        tptacek 12y ago
        You just moved the goalposts. Yes, it's suboptimal to have a server that receives a request with a 32 bit length field indicating 3 gigs of incoming data that the bombs out trying to malloc 32 gigs. I'm saying: what's an example of a server that reliably, for every allocation of every piece of metadata, every strdup, every hash table entry, every connection object, &c &c, has a recovery regime so that it doesn't have to fail ever when malloc does? One way you could find such a program would be to compile a candidate and preload a malloc that randomly (1 in every 100 calls per allocation size, for instance) returns NULL. See if the program (a) continues to run and (b) passes some simple unit test suite. My contention is: (a) Those programs will be hard to find, because most "production ready" Unix code does not have that property, and (b) The criteria I'm talking about matters a lot, because (1) memory exhaustion strikes at totally arbitrary points in a program's execution, not just at the points where you're prepared to handle it, and (2) attackers can pinpoint exactly the allocation they want to have fail. As an example of an approach that doesn't seem to work well: in "memqueue", the function that creates HTTP headers in responses allocates an array of iovecs. If that malloc fails, the header creation function returns -1. The function that calls the header-creation function, http_respond, catches that error and itself returns -1. Nothing ever checks the error return from http_respond. (Also, the loop in which memqueue reads entire requests into memory by continuously realloc()'ing a receive buffer has an integer wrap bug in it, though it's probably not triggerable. But incorrect is incorrect, right?)
halayli 12y ago
How do you know? What if someone forked this program and closed fd 0 before execX()? You have to abide by the posix standard and what the man page tells you. Now it never happens, but later the code can change and the case can hit. Most probably you'd have forgotten about this check. Malloc failures don't always have to result in aborting the program. Cases can vary. My suggestion to you is abide by the man page and always check error conditions. Don't overthink the failure cases of stdlib & posix calls. You never know the OS/environment your program will run in, and the only common ground you have is the standard.
- tptacek 12y ago
  You don't know. It's true that the line of code he cited was incorrect. It's just not a very interesting example of incorrect code. Malloc failures don't always result in aborting. The common alternative, especially in programs that have careful malloc return value checking regimes, is to occasionally cough up remote code execution. Userland systems programmers should assume the conservative default of ending the program immediately when malloc fails. A similar logic guided C++ into throwing bad_alloc instead of returning NULL on allocation failures. And a survey of modern C++ code will show you that most C++ programs simply allow themselves to terminate when bad_alloc happens.
  - halayli 12y ago
    It doesn't have to be interesting. Incorrect code covers a spectrum and is not black or white. The term "edge-case" describes it pretty well. Regarding mallocs, again you don't have to overthink error-handling. Just handle it and figure out how to deal with it if it happens. C++ has nothrow and try/catch. If you want to catch a failed new or abort is something up to the user. One thing for sure is that C++ aborts on failed new instead of stumping on unallocated memory. On the other hand, consistency is important. Some parts of the code handled fd correctly and malloc failures and other parts did not. I recommend you applaud good programming practices and criticize bad ones. Defending/arguing bad programming practices(whether they are edge cases or not) doesn't help.
    - 12y ago