> AFAIK snprintf() doesn't write the final \0 in the case the string would
> overflow the buffer, so with size 6 you might end with "foobar" instead
> of "fooba\0". I think this was the problem the patch was trying to
> remedy. But perhaps it's not an issue anyway.
I sat down and tested this, and you're correct: it won't zero-terminate
if it runs out of space to do so, so we need to do so ourselves.
I've fixed this now (ff4e6c7), and also fixed the function immediately
above it that had the same problems :P
The man page for snprintf on Debian is a bit vague/misleading about
this. Reading it, I was under the impression that it would always
zero-terminate, even if that would lead to truncation. It talks a bit
about detecting truncation, but doesn't mention that it doesn't always
zero-terminate (and, to my eyes, sort of implies that it actually does):
> DESCRIPTION:
> The functions snprintf() and vsnprintf() write at most size bytes
> (including the terminating null
> byte ('\0')) to str.
>
> Return value:
> The functions snprintf() and vsnprintf() do not write more than size
> bytes (including the terminat‐
> ing null byte ('\0')). If the output was truncated due to this limit,
> then the return value is the
> number of characters (excluding the terminating null byte) which
> would have been written to the
> final string if enough space had been available. Thus, a return value
> of size or more means that
> the output was truncated. (See also below under NOTES.)
>
> NOTES:
> (talks about overlapping src/dest buffers and return values)
>
> BUGS:
> (talks about snprintf and vsnprintf as solutions to overflow problems,
> but doesn't mention they have their own)
The man page for strncpy, which exhibits the same behaviour, talks at
length about the need to ensure zero termination yourself. Not sure if
this is an snprintf bug or a documentation bug (or both) ...
> > > 0009-Fix-PATH_MAX-on-GNU-Hurd.patch
> > [...]
> > The rest of this patch looks okay, but if we're going to compensate for
> > a missing MAXHOSTNAMELEN I'd want a reasonable value in there.
> > Thoughts? (FWIW: looks like it's 64 on Debian, 256 on OS X.)
>
> It caught my eye as well, but I haven't given it enough thought.
>
> It should definitely be:
>
> +#ifndef MAXHOSTNAMELEN
> +#define MAXHOSTNAMELEN 256
> +#endif
I've applied this one now, with MAXHOSTNAMELEN set to 256
If my reckoning is correct, that's all the patches now applied, except
for the one that's being discarded due to no longer being needed.
They aren't backported to 2.5 (or earlier) yet, but will be before the
next release.
> Thank you for the review, this will help me keep the patches we carry in
> Debian to minimum.
Thanks for sending them through! :)
Cheers,
ellie
On Fri, Sep 25, 2015, at 06:05 PM, Ondřej Surý wrote:
> Hi Ellie,
>
> On Fri, Sep 25, 2015, at 03:32, ellie timoney wrote:
> > Hi Ondřej,
> >
> > Thanks for sending these upstream. A bunch of these I can just apply,
> > and will do so today. I'll email again when they're on master. Once
> > they're on master I'll revisit them for inclusion on 2.5 and maybe 2.4.
> >
> > A few them warrant further discussion, which follows.
> >
> > Cheers,
> >
> > ellie
> >
> > > 0001-Fix-potential-buffer-overflows.patch
> >
> > I'll apply the first part of this patch (imtest/imtest.c).
> >
> > The second part of this patch (master/master.c) adds an explicit
> > 0-termination to a string that was constructed using snprintf. Is there
> > something I'm missing here? To my reading of documentation, snprintf
> > already takes care of this. Or is this a case of a stale patch fixing a
> > problem that no longer exists?
>
> AFAIK snprintf() doesn't write the final \0 in the case the string would
> overflow the buffer, so with size 6 you might end with "foobar" instead
> of "fooba\0". I think this was the problem the patch was trying to
> remedy. But perhaps it's not an issue anyway.
>
> > > 0002-Use-proper-types-uid_t-and-gid_t-instead-of-int-for-.patch
> >
> > I'll apply this one
> >
> > > 0003-Silence-erroneous-RLIMIT_NUMFDS-related-log-messages.patch
> >
> > The commit message sounds like this was trying to fix a problem that
> > we've since fixed ourselves. The changes that remain are not ideal:
> >
> > > - if (!getrlimit(RLIMIT_NUMFDS, &rl)) {
> > > + if (getrlimit(RLIMIT_NUMFDS, &rl) >= 0) {
> >
> > getrlimit() returns 0 on success, -1 on failure. The Debian patch
> > differs by also treating positive non-zero as success. Which seems
> > strange: if anything that should be a really bad error, of the
> > "something is very wrong, can't trust response from getrlimit" variety.
> >
> > > - if (setrlimit(RLIMIT_NUMFDS, &rl) < 0) {
> > > + if (setrlimit(RLIMIT_NUMFDS, &rl) < 0 && x != RLIM_INFINITY) {
> >
> > This change will silence an actual error (though we do not pass
> > RLIM_INFINITY to limit_fds() anymore, anyway, as of a few days ago).
> >
> > I don't think this patch is needed anymore.
>
> Ok, great, I will drop it as well.
>
> > > 0004-Fix-xmalloc-usage.patch
> >
> > I'll apply this one.
> >
> > > 0005-Make-TLS-SSL-error-message-more-informative.patch
> >
> > I'll apply this one, and also make the updated wording consistent (c.f
> > "may be", "maybe", "might be")
> >
> > > 0006-Minor-fixes-of-master.conf-parsing-to-be-more-verbos.patch
> >
> > I'll apply this one.
> >
> > > 0007-Fix-formatting-of-imclient-manpage.patch
> >
> > I'll apply this one, though the wording of the fix irks me for some
> > reason. But at least the formatting is correct, and we can tune the
> > wording later.
>
> :)
>
> > > 0008-Change-the-wording-of-sieved-s-notice-when-a-user-s-.patch
> >
> > I'll apply this one.
> >
> > > 0009-Fix-PATH_MAX-on-GNU-Hurd.patch
> >
> > The first part of this patch (imap/pop3d.c) is bothering me:
> >
> > > +#ifndef MAXHOSTNAMELEN
> > > +#define MAXHOSTNAMELEN
> > > +#endif
> > > +
> >
> > MAXHOSTNAMELEN is used here:
> >
> > > static char popd_apop_chal[45 + MAXHOSTNAMELEN + 1]; /*
> > > <rand.time@hostname> */
> >
> > Currently, if MAXHOSTNAMELEN is undefined, that's a compilation failure.
> >
> > This change will maybe allow compilation to succeed, in which case
> > popd_apop_chal might be too short, resulting in a misleading run time
> > error from sasl_mkchal().
> >
> > Or maybe it won't build, depending on how the empty MAXHOSTNAMELEN
> > between two additions gets interpreted, resulting in an equally
> > misleading compile time error.
> >
> > The rest of this patch looks okay, but if we're going to compensate for
> > a missing MAXHOSTNAMELEN I'd want a reasonable value in there.
> > Thoughts? (FWIW: looks like it's 64 on Debian, 256 on OS X.)
>
> It caught my eye as well, but I haven't given it enough thought.
>
> It should definitely be:
>
> +#ifndef MAXHOSTNAMELEN
> +#define MAXHOSTNAMELEN 256
> +#endif
>
> or 64?
>
> Looks like there's a confusion whether hostname is a DNS label (max 63
> octets) or DNS fqdn (max 255 octets).
>
> /usr/include/asm-generic/param.h:#define MAXHOSTNAMELEN 64 /* max
> length of hostname */
> /usr/include/protocols/timed.h:#define MAXHOSTNAMELEN 64
> /usr/include/rpc/types.h:#define MAXHOSTNAMELEN 64
> /usr/include/X11/Xtrans/Xtranssock.c:#define MAXHOSTNAMELEN 255
> /usr/include/mit-krb5/gssrpc/types.h:#define MAXHOSTNAMELEN 64
>
> > > 0010-Disable-SSLv2-SSLv3-and-TLS-compression-use-TLS_-_me.patch
> >
> > This patch explicitly disables TLS compression (good, due to
> > https://en.wikipedia.org/wiki/CRIME), regardless of what the
> > "tls_compression" setting in imapd.conf is set to (default: off)
>
> Yup.
>
> > I'll apply it, and also get rid of the setting, I guess.
>
> Oh, sorry, I forgot about killing the setting.
>
> > > 0011-libisieve-has-to-be-noinst_LTLIBRARY-for-PIC-code-to.patch
> >
> > I'll apply this one.
> >
> > > 0012-Fix-typo-in-sphinx-that-disabled-squat.patch
> >
> > I'll apply this one.
> >
> > > 0013-Change-the-configure-check-for-PS_STRINGS-to-COMPILE.patch
> >
> > I'll apply this one.
>
> Thank you for the review, this will help me keep the patches we carry in
> Debian to minimum.
>
> Cheers,
> --
> Ondřej Surý <ond...@sury.org>
> Knot DNS (https://www.knot-dns.cz/) – a high-performance DNS server
