4 ms·

I don't see anything wrong with that code. There is nothing wrong with using the stack for what it was designed for. The function names are self-explanatory.

I don't see anything wrong with that code. There is nothing wrong with using the stack for what it was designed for. The function names are self-explanatory. The "style" might not be yours, but it doesn't make it bad. And as described in the slides, it was derived from relayd.

jacquesm 12y ago
> I don't see anything wrong with that code. Look longer. Let me give you some examples: Forward declarations for functions that could be avoided by re-arranging the code, using both 0 and -1 to indicate error returns from functions, bits like: s = fd == -1 ? socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP) : fd; if (s == -1) goto bad; Sure, I can read that but it takes more effort than it should and could be re-written much clearer and so on. > There is nothing wrong with using the stack for what it was designed for. The stack should not be used to allocate buffers intended to hold data written by other routines or read from untrusted sources for reasons that have become painfully clear over the last couple of years. > The function names are self-explanatory. Strong disagree about the function names being self explanatory, plenty of the functions have non-obvious side-effects. The 'style' is asking for trouble and as for it being derived from 'relayd' that pretty much confirms that this isn't something new (which is actually a good thing), but an adaptation of something old to a new role (such adaptations are security wise something to be very wary of, re-purposing old code is a great way to find out what edge cases were missed previously). Far more symbols are exported than necessary. > The "style" might not be yours, but it doesn't make it bad. There's a return at the end of a function returning void for no reason. #if 0'd old code that should simply be purged. In the server_log code I think there may be a path to get a double free of 'ptr' where it is used first in the block with the while loop, then not reset to NULL and re-used in the second block and freed if it is not NULL (which it still is from the previous block...). The style used obscures this possibility.
- Someone 12y ago
  The assignment in the while boolean condition sets per to NULL at the end of the loop.
  - jacquesm 12y ago
    Yes, you are right. It should not take such close reading of the code though to make sure that this condition can not occur.
    - sfk 12y ago
      Since you have such valuable insights, perhaps you should discuss them on openbsd-tech.
  - Sanddancer 12y ago
    To be fair, I think that the given block could be written better both in terms of clarity and in terms of knocking out unneeded functions. s = fd == -1 ? socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP) : fd; if (s == -1) goto bad; Is the equivalent of if (fd == -1) { s = -1; goto bad; } else { s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP); } but with one less comparison, assignment of a constant to s in the error condition instead of a variable, which is usually faster, and a more clear, explicit, layout. Now, the compiler may make these optimizations for you, but it still leaves an uglier bit of code. Tristate operators can be useful, but in some cases, like this, they can make the code less efficient.
    - termie 12y ago
      Your refactor is wrong. You've reversed the ternary and removed the socket() return check. The consolation prize is that you've reinforced the issue of lack of clarity in the code.
      - Sanddancer 12y ago
        You're right. I was thrown off by them putting the exceptional condition as the first part of the ternary operation. Chalk this to overly clever code. if (fd != -1) { s = fd; } else { s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP); if (s == -1) { goto bad; } } or in rewriting the ternary function for clarity: // If we have a socket, use it, otherwise, try to get one s = (fd != -1) ? fd : socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP); if ( s == -1 ) // we still don't have a socket, error out goto bad; Ternary operators should, in my opinion, be put to the 3 am test. If you think you'd get confused at looking at code that uses one at 3 am, then you've not written it properly and should clarify.
- cremno 12y ago
  >using both 0 and -1 to indicate error returns from functions I couldn't find any function that returns 0 on failure. Or do you mean a null pointer? >The stack should not be used to allocate buffers intended to hold data written by other routines for reasons that have become painfully clear over the last couple of years. You want to see a zero tolerance policy for them? I think this is a bit overprotective. Their maximum size is usually known and all called functions take a size argument (snprintf(), strftime(), strlcpy(), etc.). There's a chance one could make a mistake here, but C is the wrong language to dynamically allocate everything. The probability to forget to free it, including in every error path, is much higher in my eyes. But then again, a memory leak is probably preferable than a buffer overflow (of course, both are bad). There are also already many heap allocated buffers. >Looking a bit longer at the server_log code I think there may be a path to get a double free of 'ptr' where it is used first in the block with the while loop, then not reset to NULL and re-used in the second block and freed if it is not NULL (which it still is from the previous block...) I don't think so, but that code really is dodgy. Also, `ptr != NULL` is unnecessary (just mentioning since OpenBSD/LibreSSL developers like to mention that too).
  - jacquesm 12y ago
    > I couldn't find any function that returns 0 on failure. Or do you mean a null pointer? server_socket_getport An unknown address family will return a '0'. > You want to see a zero tolerance policy for them? No, but if you use that mechanism then it is preferable to have all the functions hitting that buffer to be visible from the scope of the declaration of the buffer. Passing it on to other libraries can cause problems when/if those libraries' maintainers mess up. So if you're defensive about this then you can't be hurt that way. > I don't think so, Agreed, looking still longer it looks like it will always end up with a NULL in it after the while, but that's very ugly to put it mildly. > Also, `ptr != NULL` is unnecessary (just mentioning since OpenBSD/LibreSSL developers like to mention that too). Yep. And there are plenty of other nitpicks like that but I don't even care that much about any of those, I mostly care about the way that the code is laid out making it an excellent place to hide some really nasty bugs.
    - cremno 12y ago

jacquesm 12y ago

> I don't see anything wrong with that code. Look longer. Let me give you some examples: Forward declarations for functions that could be avoided by re-arranging the code, using both 0 and -1 to indicate error returns from functions, bits like: s = fd == -1 ? socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP) : fd; if (s == -1) goto bad; Sure, I can read that but it takes more effort than it should and could be re-written much clearer and so on. > There is nothing wrong with using the stack for what it was designed for. The stack should not be used to allocate buffers intended to hold data written by other routines or read from untrusted sources for reasons that have become painfully clear over the last couple of years. > The function names are self-explanatory. Strong disagree about the function names being self explanatory, plenty of the functions have non-obvious side-effects. The 'style' is asking for trouble and as for it being derived from 'relayd' that pretty much confirms that this isn't something new (which is actually a good thing), but an adaptation of something old to a new role (such adaptations are security wise something to be very wary of, re-purposing old code is a great way to find out what edge cases were missed previously). Far more symbols are exported than necessary. > The "style" might not be yours, but it doesn't make it bad. There's a return at the end of a function returning void for no reason. #if 0'd old code that should simply be purged. In the server_log code I think there may be a path to get a double free of 'ptr' where it is used first in the block with the while loop, then not reset to NULL and re-used in the second block and freed if it is not NULL (which it still is from the previous block...). The style used obscures this possibility.

Someone 12y ago
The assignment in the while boolean condition sets per to NULL at the end of the loop.
- jacquesm 12y ago
  Yes, you are right. It should not take such close reading of the code though to make sure that this condition can not occur.
  - sfk 12y ago
    Since you have such valuable insights, perhaps you should discuss them on openbsd-tech.
- Sanddancer 12y ago
  To be fair, I think that the given block could be written better both in terms of clarity and in terms of knocking out unneeded functions. s = fd == -1 ? socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP) : fd; if (s == -1) goto bad; Is the equivalent of if (fd == -1) { s = -1; goto bad; } else { s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP); } but with one less comparison, assignment of a constant to s in the error condition instead of a variable, which is usually faster, and a more clear, explicit, layout. Now, the compiler may make these optimizations for you, but it still leaves an uglier bit of code. Tristate operators can be useful, but in some cases, like this, they can make the code less efficient.
  - termie 12y ago
    Your refactor is wrong. You've reversed the ternary and removed the socket() return check. The consolation prize is that you've reinforced the issue of lack of clarity in the code.
    - Sanddancer 12y ago
      You're right. I was thrown off by them putting the exceptional condition as the first part of the ternary operation. Chalk this to overly clever code. if (fd != -1) { s = fd; } else { s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP); if (s == -1) { goto bad; } } or in rewriting the ternary function for clarity: // If we have a socket, use it, otherwise, try to get one s = (fd != -1) ? fd : socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP); if ( s == -1 ) // we still don't have a socket, error out goto bad; Ternary operators should, in my opinion, be put to the 3 am test. If you think you'd get confused at looking at code that uses one at 3 am, then you've not written it properly and should clarify.
cremno 12y ago
>using both 0 and -1 to indicate error returns from functions I couldn't find any function that returns 0 on failure. Or do you mean a null pointer? >The stack should not be used to allocate buffers intended to hold data written by other routines for reasons that have become painfully clear over the last couple of years. You want to see a zero tolerance policy for them? I think this is a bit overprotective. Their maximum size is usually known and all called functions take a size argument (snprintf(), strftime(), strlcpy(), etc.). There's a chance one could make a mistake here, but C is the wrong language to dynamically allocate everything. The probability to forget to free it, including in every error path, is much higher in my eyes. But then again, a memory leak is probably preferable than a buffer overflow (of course, both are bad). There are also already many heap allocated buffers. >Looking a bit longer at the server_log code I think there may be a path to get a double free of 'ptr' where it is used first in the block with the while loop, then not reset to NULL and re-used in the second block and freed if it is not NULL (which it still is from the previous block...) I don't think so, but that code really is dodgy. Also, `ptr != NULL` is unnecessary (just mentioning since OpenBSD/LibreSSL developers like to mention that too).
- jacquesm 12y ago
  > I couldn't find any function that returns 0 on failure. Or do you mean a null pointer? server_socket_getport An unknown address family will return a '0'. > You want to see a zero tolerance policy for them? No, but if you use that mechanism then it is preferable to have all the functions hitting that buffer to be visible from the scope of the declaration of the buffer. Passing it on to other libraries can cause problems when/if those libraries' maintainers mess up. So if you're defensive about this then you can't be hurt that way. > I don't think so, Agreed, looking still longer it looks like it will always end up with a NULL in it after the while, but that's very ugly to put it mildly. > Also, `ptr != NULL` is unnecessary (just mentioning since OpenBSD/LibreSSL developers like to mention that too). Yep. And there are plenty of other nitpicks like that but I don't even care that much about any of those, I mostly care about the way that the code is laid out making it an excellent place to hide some really nasty bugs.
  - cremno 12y ago