deflatePending not available in zlib on OpenBSD (undefined symbol)

[Ken Murchison]

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