Hi Ken, Is there anything preventing the merge of your change in the PR?
I thought it would be included in 3.2.2. Regards, Anatoli On 3/6/20 17:47, Ken Murchison wrote: > This is my latest proposed fix: > https://github.com/cyrusimap/cyrus-imapd/pull/3061 > > > On 6/2/20 7:34 PM, Anatoli wrote: >> Looks good to me and compiles correctly on OpenBSD. >> >> Could it be included in the next 3.2 release (3.2.2)? >> >> >> On 2/6/20 19:31, Ken Murchison wrote: >>> Yes, you're correct. We have a comment in prot.c that quotes the same >>> passage ion the docs. >>> >>> I think this might do the trick: >>> >>> >>> diff --git a/imap/httpd.c b/imap/httpd.c >>> index fc430d935..b5014b97e 100644 >>> --- a/imap/httpd.c >>> +++ b/imap/httpd.c >>> @@ -149,24 +149,12 @@ HIDDEN int zlib_compress(struct transaction_t >>> *txn, unsigned flags, >>> zstrm->next_in = (Bytef *) buf; >>> zstrm->avail_in = len; >>> >>> - buf_ensure(&txn->zbuf, deflateBound(zstrm, zstrm->avail_in)); >>> buf_reset(&txn->zbuf); >>> >>> do { >>> int zr; >>> >>> - if (!zstrm->avail_out) { >>> - unsigned pending; >>> - >>> - zr = deflatePending(zstrm, &pending, Z_NULL); >>> - if (zr != Z_OK) { >>> - /* something went wrong */ >>> - syslog(LOG_ERR, "zlib deflate error: %d %s", zr, >>> zstrm->msg); >>> - return -1; >>> - } >>> - >>> - buf_ensure(&txn->zbuf, pending); >>> - } >>> + buf_ensure(&txn->zbuf, deflateBound(zstrm, len)); >>> >>> zstrm->next_out = (Bytef *) txn->zbuf.s + txn->zbuf.len; >>> zstrm->avail_out = txn->zbuf.alloc - txn->zbuf.len; >>> >>> >>> On 6/2/20 6:19 PM, Anatoli wrote: >>>> Hi Ken, >>>> >>>> According to the docs [1]: If deflate returns Z_OK and with zero >>>> avail_out, it must be called again after making room in the buffer >>>> because there might be more output pending. >>>> >>>> On the other hand it says: Z_FINISH can be used in the first deflate >>>> call after deflateInit if all the compression is to be done in a single >>>> step. In order to complete in one call, avail_out must be at least the >>>> value returned by deflateBound. Then deflate is guaranteed to return >>>> Z_STREAM_END. >>>> >>>> But: Note that it is possible for the compressed size to be larger than >>>> the value returned by deflateBound() if flush options other than >>>> Z_FINISH or Z_NO_FLUSH are used. >>>> >>>> >>>> At the beginning of zlib_compress() there's code that decides with which >>>> flush option to call deflate(). >>>> >>>> /* Only flush for static content or on last (zero-length) chunk */ >>>> >>>> If it's possible to make flush to be always Z_FINISH, then I guess the >>>> entire >>>> >>>> do { ... } while (!zstrm->avail_out); >>>> >>>> loop could be simplified to just: >>>> >>>> zstrm->next_out = (Bytef *) txn->zbuf.s + txn->zbuf.len; >>>> zstrm->avail_out = txn->zbuf.alloc - txn->zbuf.len; >>>> >>>> zr = deflate(zstrm, flush); >>>> if (zr != Z_STREAM_END) { >>>> /* something went wrong */ >>>> syslog(LOG_ERR, "zlib deflate error: %d %s", zr, zstrm->msg); >>>> return -1; >>>> } >>>> >>>> >>>> I changed the "if (zr)" condition as the only good value would be >>>> Z_STREAM_END. >>>> >>>> But if the flush option can't be always Z_FINISH, then I believe the >>>> loop should be kept with the checks for !zstrm->avail_out as it's >>>> possible that there would be not enough buffer for deflate() to complete >>>> in one call. >>>> >>>> Regards, >>>> Anatoli >>>> >>>> [1] https://www.zlib.net/manual.html >>>> >>>> >>>> >>>> On 2/6/20 17:36, Ken Murchison wrote: >>>>> Hi Anatoli, >>>>> >>>>> Thanks for the report. I'm not sure that we even need the >>>>> deflatePending() call, since we use deflateBound() to create an >>>>> appropriately-sized buffer to hold the entire compressed response body. >>>>> Let me do some testing. >>>>> >>>>> >>>>> On 6/2/20 3:48 AM, Anatoli wrote: >>>>>> Cyrus developers, >>>>>> >>>>>> Is it possible to somehow rework the code in imap/httpd.c lines 158-169 >>>>>> in order to NOT use deflatePending as this func is not available on >>>>>> OpenBSD? The zlib version there is 1.2.3 and deflatePending appeared in >>>>>> 1.2.5 so the code doesn't compile with --enable-http (undefined symbol: >>>>>> deflatePending). The packaged version disables http for now. >>>>>> >>>>>> Is it safe to reduce these lines: >>>>>> >>>>>> 158 if (!zstrm->avail_out) { >>>>>> 159 unsigned pending; >>>>>> 160 >>>>>> 161 zr = deflatePending(zstrm, &pending, Z_NULL); >>>>>> 162 if (zr != Z_OK) { >>>>>> 163 /* something went wrong */ >>>>>> 164 syslog(LOG_ERR, "zlib deflate error: %d %s", zr, >>>>>> zstrm->msg); >>>>>> 165 return -1; >>>>>> 166 } >>>>>> 167 >>>>>> 168 buf_ensure(&txn->zbuf, pending); >>>>>> 169 } >>>>>> >>>>>> >>>>>> to something like: >>>>>> >>>>>> 158 if (!zstrm->avail_out) { >>>>>> 159 buf_ensure(&txn->zbuf, 256 * 1024); >>>>>> 160 } >>>>>> >>>>>> If I understand it correctly, deflatePending in this case is only used >>>>>> to get the needed buffer size and we could replace it with a hardcoded >>>>>> buffer size (like in my example of 256K). >>>>>> >>>>>> >>>>>> Thanks, >>>>>> Anatoli >
