Debian patches that really belong to upstream

[ellie timoney]

Hi again,

> > 0001-Fix-potential-buffer-overflows.patch
> 
> I'll apply the first part of this patch (imtest/imtest.c).

Done,

> 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?

I think the second part actually introduces a potential future buffer
overflow: if this function is ever called with size=0, then size-1 will
underflow, and path[size-1] = '\0' will try to write to path[UINT_MAX].

> > 0002-Use-proper-types-uid_t-and-gid_t-instead-of-int-for-.patch
> 
> I'll apply this one

Done. I also needed to remove some int casts from calls to getuid() et
al around line 610 to make this compile with -Werror

> > 0004-Fix-xmalloc-usage.patch
> 
> I'll apply this one.

Done

> > 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")

Done

> > 0006-Minor-fixes-of-master.conf-parsing-to-be-more-verbos.patch
> 
> I'll apply this one.

Done, and fixed whitespace

> > 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.

Done

> > 0008-Change-the-wording-of-sieved-s-notice-when-a-user-s-.patch
> 
> I'll apply this one.

Done, and fixed whitespace

> > 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)
> 
> I'll apply it, 

Done, and also removed a (now useless) check of IMAPOPT_TLS_COMPRESSION
from tls_init_clientengine(), similarly to the one removed from
tls_init_serverengine()

> and also get rid of the setting, I guess.

... and done

> > 0011-libisieve-has-to-be-noinst_LTLIBRARY-for-PIC-code-to.patch
> 
> I'll apply this one.

Done

> > 0012-Fix-typo-in-sphinx-that-disabled-squat.patch
> 
> I'll apply this one.

Done

> > 0013-Change-the-configure-check-for-PS_STRINGS-to-COMPILE.patch
> 
> I'll apply this one.

Done

On Fri, Sep 25, 2015, at 11:32 AM, 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?
> 
> > 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.
> 
> > 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.)
> 
> > 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)
> 
> I'll apply it, and also get rid of the setting, I guess.
> 
> > 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.
> 
> On Tue, Sep 22, 2015, at 11:07 PM, Ondřej Surý wrote:
> > Hi,
> > 
> > [ccing Brong since my last attempt to send this was rejected by mailman]
> > 
> > I was adapting Debian packaging for 3.0.0-beta1 and I have encountered
> > several patches that really belong to upstream git.
> > 
> > You can find them here (rebased on top of the latest master):
> > https://github.com/oerdnj/cyrus-imapd
> > 
> > or attached to this email (using git format-patch, so you can feed them
> > to git am)
> > 
> > Some of those are really old, so perhaps they doesn't have to be
> > applied, but I picked only those that looked sane and still make sense.
> > Most of those patches are small fixes, nothing huge, and I tried to keep
> > the original author in the commit if known.
> > 
> > The Debian packaging is under the same license as the original Cyrus, so
> > no license worries here.
> > 
> > 0011 and 0012 only apply to master branch (as they are bugs only in
> > 3.0.0), but the rest of the patches could be applied on 2.4.x and
> > perhaps 2.5.x (I decided to skip 2.5.x packaging in favour of 3.0.0).
> > 
> > If you need more explanation for any of those patches, I will be happy
> > to provide an explanation.
> > 
> > Cheers,
> > -- 
> > Ondřej Surý <ondrej at sury.org>
> > Knot DNS (https://www.knot-dns.cz/) – a high-performance DNS server
> > Email had 13 attachments:
> > + 0013-Change-the-configure-check-for-PS_STRINGS-to-COMPILE.patch
> >   2k (text/x-patch)
> > + 0012-Fix-typo-in-sphinx-that-disabled-squat.patch
> >   1k (text/x-patch)
> > + 0011-libisieve-has-to-be-noinst_LTLIBRARY-for-PIC-code-to.patch
> >   3k (text/x-patch)
> > + 0010-Disable-SSLv2-SSLv3-and-TLS-compression-use-TLS_-_me.patch
> >   7k (text/x-patch)
> > + 0009-Fix-PATH_MAX-on-GNU-Hurd.patch
> >   2k (text/x-patch)
> > + 0008-Change-the-wording-of-sieved-s-notice-when-a-user-s-.patch
> >   2k (text/x-patch)
> > + 0007-Fix-formatting-of-imclient-manpage.patch
> >   1k (text/x-patch)
> > + 0006-Minor-fixes-of-master.conf-parsing-to-be-more-verbos.patch
> >   2k (text/x-patch)
> > + 0005-Make-TLS-SSL-error-message-more-informative.patch
> >   3k (text/x-patch)
> > + 0004-Fix-xmalloc-usage.patch
> >   2k (text/x-patch)
> > + 0003-Silence-erroneous-RLIMIT_NUMFDS-related-log-messages.patch
> >   2k (text/x-patch)
> > + 0002-Use-proper-types-uid_t-and-gid_t-instead-of-int-for-.patch
> >   1k (text/x-patch)
> > + 0001-Fix-potential-buffer-overflows.patch
> >   2k (text/x-patch)