deflatePending not available in zlib on OpenBSD (undefined symbol)

[Ken Murchison]

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