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
