fix usage of most POSIX functions

[DerDakon]

• if standard functions are used, use the correct header(s) for them
• some reimplemented functions can easily be replaced with their equivalents from the C standard, which will usually result in much better code being generated by the compiler

There are still some functions left where the proper header is not included, e.g. rename(). This function is declared in <stdio.h>, which would collide badly with the custom puts() functions used at multiple places.

[alanpost]

The "get rid of" series in this PR should be submitted separately. The set of steps I need to perform for due diligence differ for that series than from the other commits here, due to at least "4.1 Identifying common functions" in djb's "Some thoughts on security after ten years of qmail 1.0."

That question doesn't even come up for the other patches, hence my request to file separate PRs.

[alanpost]

Would you please pull these commits in to separate PRs:

add missing return types to main()
use ssize_t in substdio

the main return types I have some comments on and would like to see merged shortly. The bottom one is likely going to be necessary on at least one platform, now that #80 is in, and I'd like to give it a similar separate treatment (comments and corrections).

[DerDakon]

Done in #84 and #85.

[schmonz]

In-tree, byte_zero() is called from three places:

1. qmail-popup.c, with the intent of clearing username/password/timestamp from memory ASAP
2. timeoutconn.c and remoteinfo.c, to zero out a new struct

For (2), I'm less worried about what compilers will optimize out; this looks like an idiomatic use of memset().

For (1), I'm more worried. How can we tell whether the existing hand-rolled byte_zero() implementation has the desired effect, and whether a memset() preserves the expected behavior?

Out of tree, I know of at least my own authup program that's derived from qmail-popup and has the same expectations for byte_zero().

[schmonz]

Trivial Q about include ordering, and a bigger one about reliably zeroing out sensitive memory.

I'm not getting warnings without the missing includes, probably because the system header is already included transitively. I'm in favor of direct inclusion, though, so as usual I'm curious how you found these (and whether it's something we can/should automate).

[DerDakon]

I'm not getting warnings without the missing includes, probably because the system header is already included transitively. I'm in favor of direct inclusion, though, so as usual I'm curious how you found these (and whether it's something we can/should automate).

Building with -Wall, probably -Wmissing-declarations is enough. That these errors now are not happening is very likely the result of other work that introduced <unistd.h> in some of our headers (e.g. readwrite.h), where it was originally also missing. I still think that we should add them explicitly to these *.c files where we use getpid() or other functions directly.

I have no problem dropping the byte_zero() change from this commit for now and keep it for as long as qmail-popup.c exists, or until we find a better way. The other 2 users could be solved differently if we actually care.

[josuah]

Some more suggestions that could jump onto that merge, but otherwise all in favor of this.

[josuah]

For example, in qmail-local.c: byte_copy(fnnewtph,3,"new");

So:

#define byte_copy(d,n,s) (memcpy(d,s,n))

Or to preserve semantics:

#define byte_copy(d,n,s) (memmove(d,s,n))

[josuah]

The memrchr is a widely available extension, only memchr is (C89) standard.

[DerDakon]

So what we have left are 5 commits, 4 of them add sort-of missing headers, and then the somewhat dangerous memset thing. This has been much more at the start, and more and more parts of it have been done through dedicated MRs, and I think it's about time to split the 4 header commits into an own MR, and let the memset() on rest in piece here.

[DerDakon]

The relevant commits have been moved to #237, I think the remainder can be dropped.

• byte_zero() is kept to make it more likely that the zeroing of the password buffer is not optimized out
• <sys/types.h> is not really needed for umask() as no variable is used for the return value or the argument