I'd like to get it tested by someone other than me and probably under load.
On 6/22/20 4:28 PM, Anatoli wrote:
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
--
Kenneth Murchison
Senior Software Developer
Fastmail US LLC
