deflatePending not available in zlib on OpenBSD (undefined symbol)

[Ken Murchison]

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