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@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ý <ond...@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)
