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.
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;
> 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.
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).
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.
I found this interesting article: http://daniel.haxx.se/blog/2014/10/25/pretending-port-zero-i...
So since 0 can be okay and (in_port_t)-1 (=65535) is okay, it has to be changed to return int and take a in_port_t* to be acceptable for you. The return value may not even matter if only AF_INET/AF_INET6 as family is possible - a comment could be of help here.
In this particular case, returning 0 doesn't necessarily indicate failure. Binding a socket to port 0 means you're asking the operating system to pick an available port for you, which one might argue is a reasonably safe default for unknown address families.
Let it crash, as close as possible to the point of origin of a problem is a very good principle.
There's a trick you can use to make remembering to free it more likely. Some people call it "RAII in C", I like to think of it as "nested allocations and errors": When you allocate something, or do something else which one, can fail, and two, must be undone (freed, closed, etc.), do the allocation and deallocation in properly nested pairs, and use gotos to jump to the code which undoes the last successful allocation if some allocation goes wrong. Basically, you arrange your code in a stack, where the most recent successful allocation is undone first, so you can neatly jump down to the code which undoes all successful allocations and doesn't try to undo the unsuccessful ones.
For example:
int foo(void)
{
int ret = 0; /* Variable we return to indicate
success or failure. Defaults to 0,
which is success. */
FILE *inf;
FILE *outf;
if ((inf = fopen("foo", "rb")) == NULL) {
ret = -1;
goto inf_fail;
}
if ((outf = fopen("bar", "wb")) == NULL) {
ret = -2;
goto outf_fail;
}
/* Do the actual work, now that you know you have
all the resources you need. */
fclose(outf);
outf_fail:
fclose(inf); /* If we jump here, we know we
successfully opened inf,
but not outf. */
inf_fail: /* If we jump here, we didn't actually
open squat, so all we can do is return
the status variable we set above. */
return ret;
}
int foo(void)
{
int ret = -1; /* Variable we return to indicate
success or failure. Defaults to -1,
which is failure. */
FILE *inf;
FILE *outf;
if (!(inf = fopen("foo", "rb"))) {
ret = -1;
return ret;
}
do {
if (!(outf = fopen("bar", "wb"))) {
ret = -2;
break;
}
do {
/* Do the actual work, now that you know you have
all the resources you need.
Use break if something fails.
*/
ret = 0;
} while(0);
fclose(outf);
} while(0);
fclose(inf);
return ret;
}
int foo(void)
{
int ret = 0; /* Variable we return to indicate
success or failure. Defaults to 0,
which is success. */
FILE *inf = NULL;
FILE *outf = NULL;
if ((inf = fopen("foo", "rb")) == NULL) {
ret = -1;
goto foo_fail;
}
if ((outf = fopen("bar", "wb")) == NULL) {
ret = -2;
goto foo_fail;
}
/* Do the actual work, now that you know you have
all the resources you need. */
foo_fail:
if (outf) {
fclose(outf);
outf = NULL;
}
if (inf) {
fclose(inf);
inf = NULL;
}
return ret;
}
s = fd == -1 ? socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP) : fd;
if (s == -1)
goto bad;
if (fd == -1)
{
s = -1;
goto bad;
}
else
{
s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP);
}
if (fd != -1)
{
s = fd;
}
else
{
s = socket(ss->ss_family, SOCK_STREAM, IPPROTO_TCP);
if (s == -1)
{
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.