deflatePending not available in zlib on OpenBSD (undefined symbol)

[Anatoli]

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
>