ovsqlite (zlib edge-cases)

[Julien ÉLIE]

Hi Bo,

A suggestion for close_db():

#ifdef HAVE_ZLIB
     if (use_compression) {
         inflateEnd(&inflation);
         deflateEnd(&deflation);
         buffer_free(flate);
     }
#endif


As for zlib compression, did you happen to see failures during your 
tests?  Maybe not easily reproducible with "tiny" data like overview but 
I think some codes are not totally handled.
Z_STREAM_END does not mean processing compressed data is finished.


 From ovsqlite-server.c:

         status = deflate(&deflation, Z_FINISH);
         flate->left = (char *)deflation.next_out-flate->data;
         if (status==Z_STREAM_END) {
             overview = (uint8_t *)flate->data;
             overview_len = flate->left;
         } else {
             /* This is safe; it overwrites the last byte of the overview
                length, which we have already unpacked. */
             *--overview = 0;
             overview_len++;
         }
         deflation.next_in = NULL;
         deflation.avail_in = 0;



         status = inflate(&inflation, Z_FINISH);
         flate->left = (char *)inflation.next_out-flate->data;
         inflation.next_in = NULL;
         inflation.avail_in = 0;
         inflateReset(&inflation);
         if (status != Z_STREAM_END)
             goto corrupted;


According to <https://zlib.net/manual.html>:

"If the parameter flush is set to Z_FINISH, pending input is processed, 
pending output is flushed and deflate returns with Z_STREAM_END if there 
was enough output space. If deflate returns with Z_OK or Z_BUF_ERROR, 
this function must be called again with Z_FINISH and more output space 
(updated avail_out) but no more input data, until it returns with 
Z_STREAM_END or an error. After deflate has returned Z_STREAM_END, the 
only possible operations on the stream are deflateReset or deflateEnd.

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 
(see below). Then deflate is guaranteed to return Z_STREAM_END. If not 
enough output space is provided, deflate will not return Z_STREAM_END, 
and it must be called again as described above."

=> As deflateBound() is not used, it is not guaranteed that the 
operation totally finished in one call.  I suspect a latent bug then, 
though probably rare.



"inflate() returns Z_OK if some progress has been made (more input 
processed or more output produced), Z_STREAM_END if the end of the 
compressed data has been reached and all uncompressed output has been 
produced, Z_NEED_DICT if a preset dictionary is needed at this point, 
Z_DATA_ERROR if the input data was corrupted (input stream not 
conforming to the zlib format or incorrect check value, in which case 
strm->msg points to a string with a more specific error), Z_STREAM_ERROR 
if the stream structure was inconsistent (for example next_in or 
next_out was Z_NULL, or the state was inadvertently written over by the 
application), Z_MEM_ERROR if there was not enough memory, Z_BUF_ERROR if 
no progress was possible or if there was not enough room in the output 
buffer when Z_FINISH is used."

=> A response code different than Z_STREAM_END does not necessarily mean 
data is wrong.  Z_OK could have been returned.


FWIW, I came up with the following logic in nnrpd/line.c and 
nnrpd/nnrpd.c when implementing the NNTP COMPRESS command.  It appeared 
to work fine as far as I know but, who knows, maybe it also has other 
latent bugs.  zlib is pretty tricky...  Here is the opportunity to share 
best practices and improve our code.


         do {
             /* Grow the output buffer if needed. */
             if (zstream_out->avail_out == 0) {
                 size_t newsize = zbuf_out_size * 2;
                 zbuf_out = xrealloc(zbuf_out, newsize);
                 zstream_out->next_out = zbuf_out + zbuf_out_size;
                 zstream_out->avail_out = zbuf_out_size;
                 zbuf_out_size = newsize;
             }

             r = deflate(zstream_out,
                         zstream_flush_needed ? Z_PARTIAL_FLUSH : 
Z_NO_FLUSH);

             if (!(r == Z_OK || r == Z_BUF_ERROR || r == Z_STREAM_END)) {
                 sysnotice("deflate() failed: %d; %s", r,
                           zstream_out->msg != NULL ? zstream_out->msg :
                           "no detail");
                 return;
             }
         } while (r == Z_OK && zstream_out->avail_out == 0);




     do {
         if (zstream_in->avail_in > 0 || zstream_inflate_needed) {
             int r;

             zstream_in->next_out = p;
             zstream_in->avail_out = len;

             r = inflate(zstream_in, Z_SYNC_FLUSH);

             if (!(r == Z_OK || r == Z_BUF_ERROR || r == Z_STREAM_END)) {
                 sysnotice("inflate() failed: %d; %s", r,
                           zstream_in->msg != NULL ? zstream_in->msg :
                           "no detail");
                 n = -1;
                 break;
             }

             /* Check whether inflate() has finished to process its input.
              * If not, we need to call it again, even though avail_in 
is 0. */
             zstream_inflate_needed = (r != Z_STREAM_END);

             if (zstream_in->avail_out < len) {
                 /* Some data has been uncompressed.  Treat it now. */
                 n = len - zstream_in->avail_out;
                 break;
             }
             /* If we reach here, then it means that inflate() needs more
              * input, so we go on reading data on the wire. */
         }



-- 
Julien ÉLIE

« Sol lucet omnibus. »