-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix usage of most POSIX functions #65
base: main
Are you sure you want to change the base?
Conversation
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. |
Would you please pull these commits in to separate PRs: add missing return types to main() 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). |
b557d2f
to
514204f
Compare
bff7348
to
3df6536
Compare
3df6536
to
611786c
Compare
611786c
to
e8def7a
Compare
e8def7a
to
5893aeb
Compare
8b158be
to
4e52b09
Compare
4e52b09
to
ca02043
Compare
ca02043
to
a053d03
Compare
|
||
#define byte_zero(s,n) (memset((s),0,(n))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In-tree, byte_zero()
is called from three places:
qmail-popup.c
, with the intent of clearing username/password/timestamp from memory ASAPtimeoutconn.c
andremoteinfo.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()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
a053d03
to
2046afe
Compare
2046afe
to
a2f787e
Compare
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 I have no problem dropping the |
a2f787e
to
34964a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more suggestions that could jump onto that merge, but otherwise all in favor of this.
@@ -7,8 +7,8 @@ extern unsigned int byte_chr(); | |||
extern unsigned int byte_rchr(); | |||
extern void byte_copy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))
@@ -7,8 +7,8 @@ extern unsigned int byte_chr(); | |||
extern unsigned int byte_rchr(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memrchr
is a widely available extension, only memchr
is (C89) standard.
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. |
The relevant commits have been moved to #237, I think the remainder can be dropped.
|
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.