Debian patches that really belong to upstream

[Ondřej Surý]

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 at 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ý <ondrej at sury.org>
Knot DNS (https://www.knot-dns.cz/) – a high-performance DNS server