Debian patches that really belong to upstream

ellie timoney ellie at fastmail.com
Thu Sep 24 21:32:54 EDT 2015


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)


More information about the Cyrus-devel mailing list