bugs in file_descriptor and gzip-zlib-bzip2 filters #17

yanikibo · 2015-09-14T12:27:42Z

zlib: recover Z_BUF_ERROR conditions
file_descriptor: openmode compatibility to C++ standard

zlib_error::check throws on Z_BUF_ERROR

Openmode compatibility to C++ standard

int buffer_size -> std::streamsize buffer_size

eldiener · 2016-12-04T12:01:31Z

include/boost/iostreams/filter/zlib.hpp

@@ -347,7 +347,7 @@ bool zlib_compressor_impl<Alloc>::filter
    int result = xdeflate(flush ? zlib::finish : zlib::no_flush);
    after(src_begin, dest_begin, true);
    zlib_error::check BOOST_PREVENT_MACRO_SUBSTITUTION(result);
-    return result != zlib::stream_end; 
+    return result == zlib::okay; 


According to the zlib docs, if zlib::finish is passed in the call to deflate then zlib::stream_end is returned when successful, otherwise zlib::okay is returned when successful. I do not think your change reflects that reality.

It's for handling Z_BUF_ERROR condition.
If you analyze the code carrefully, you can see that the return value of the filter function is used to flush the buffer, not to check the stream_end.
If we treat Z_BUF_ERROR as an error and throw an exception, then (result != zlib::stream_end) == (result == zlib::okay), because the result may only have two values (okay or stream_end). So in that case the patch is ok.
If we do not treat Z_BUF_ERROR as an error, then we should return false to flush the buffer.
In that case:
symmetric_filter::write flushes the buffer according to the return value of this function, and continue until the real end of stream is reached.
symmetric_filter::read returns by the size of the data read, not by EOF(-1). Thus the user will be able to call this function again by providing a buffer big enough.

Looking at the explanation for 'filter' in boost/iostreams/filter/symmetric.hpp it does not seem that a return of Z_BUF_ERROR from calling xdeflate can be handled properly. Since we return false in that case, rather than throwing an exception as previously, doesn't that mean that iostreams treats the operations as completing successfully where it really needs to be called again. At least throwing the exception alerts the end-user to the buffer problem whereas with your change any buffer problem is masked by your handling of the Z_BUF_ERROR condition.

It's not always a user-buffer error, conversely it's an internal buffer error usually. You can get this error with a very big user buffer. It's meaningless to throw an exception for an internal recoverable error. I use this library intensively for io operations with high throughput, and this fix really works for Z_BUF_ERROR conditions.

Since we return false in that case, rather than throwing an exception as previously, doesn't that mean that iostreams treats the operations as completing successfully where it really needs to be called again.
No it does not. Because the filter function does not guarantee to complete the job in a single call, instead the outer functions check the end of stream and continue to call this function if it is not reached. Also user should know that the read and write functions does not guarantee to complete the job in a single call due to buffer issues. User should always check the completeness. So that it's meaningless to throw an exception for a situation already have to be known.

The read and write functions of iostreams classes is designed to return the size of the data operated. It informs the user by an extra call returning -1 after the operation is completed.

OK, I agree with your analysis. I would still like to see, in place of result == zlib::okay the specific test for end-of-stream and buffer error in the form of result != zlib::stream_end && result != zlib::buf_error instead. The same goes for your second case below. I am being as conservative as I can be in this change so as to invoke only the particular consequences by which a Z_BUF_ERROR is now handled differently. In other words currently not reaching the end-of-stream returns 'true', and any other result returns 'false' while with your fix I want to have it explicitly changed so that not reaching the end-of-stream and not having a buffer error returns 'true', and any other result returns 'false'.

The right way is to return (result == zlib::okay) because when we return true, the symmetric_filter continues filtering. However if there is an error (not throwing exception), it should stop filtering and return immediately. By this fix, there is only 3 values not throwing exception (Z_OK, Z_STREAM_END, Z_BUF_ERROR).
If we decide to add another value (e.g. Z_MEM_ERROR) to this noexception list, the filter function shall return false for that error code too, otherwise the symmetric_filter will enter an infinite loop.
So that returning (result == zlib::okay) is the right way.

zlib::okay means that the operation is successfull and there is more data to be processed. This is exactly what symmetric_filter expects to continue filtering.

eldiener · 2016-12-04T12:08:44Z

include/boost/iostreams/filter/zlib.hpp

@@ -381,7 +381,8 @@ bool zlib_decompressor_impl<Alloc>::filter
    int result = xinflate(zlib::sync_flush);
    after(src_begin, dest_begin, false);
    zlib_error::check BOOST_PREVENT_MACRO_SUBSTITUTION(result);
-    return !(eof_ = result == zlib::stream_end);
+    eof_ = result == zlib::stream_end;
+    return result == zlib::okay;


I do not think your change here is correct as it follows the same logic as when xdeflate was called.

The return value is used to flush the buffer, not to check the stream_end. Instead the eof_ variable is used to check the end of stream case.

eldiener · 2016-12-04T12:12:20Z

src/zlib.cpp

@@ -76,7 +76,7 @@ void zlib_error::check BOOST_PREVENT_MACRO_SUBSTITUTION(int error)
    switch (error) {
    case Z_OK: 
    case Z_STREAM_END: 
-    //case Z_BUF_ERROR: 
+    case Z_BUF_ERROR: 


Please explain why you think this change should be made. You are eseentially saying that a buffer error should not throw an exception.

Z_BUF_ERROR is not a fatal error, it's a recoverable error. If we treat it as an error and throw an exception, we can not recover it. To recover this error, we should flush the data, empty the buffer and retry.

5. deflate() or inflate() returns Z_BUF_ERROR:

Before making the call, make sure that avail_in and avail_out are not zero. When setting the parameter flush equal to Z_FINISH, also make sure that avail_out is big enough to allow processing all pending input. Note that a Z_BUF_ERROR is not fatal--another call to deflate() or inflate() can be made with more input or output space. A Z_BUF_ERROR may in fact be unavoidable depending on how the functions are used, since it is not possible to tell whether or not there is more output pending when strm.avail_out returns with zero.
Reference: http://www.zlib.net/zlib_faq.html#faq05

eldiener · 2016-12-06T00:32:36Z

The file_descriptor change broke the file descriptor tests so i am going to revert the file descriptor changes you made until I can look at them more closely and sync them with the file descriptor tests.

eldiener · 2016-12-06T19:29:17Z

Some other change broke the gzip test, so I have backed out all the changes. I will be testing them out individually and re-instating them individually with any necessary fixes. I do thank you for this PR, even if it did not work out.

yanikibo added 3 commits September 14, 2015 15:17

Ticket #7810

82e5005

zlib_error::check throws on Z_BUF_ERROR

Ticket #7810

2d9e91b

zlib_error::check throws on Z_BUF_ERROR

Update file_descriptor.cpp

1fd38cd

Openmode compatibility to C++ standard

yanikibo changed the title ~~Ticket #7810~~ bugs in file_descriptor and gzip-zlib-bzip2 filters Sep 29, 2015

yanikibo and others added 5 commits September 29, 2015 16:07

Update zlib.hpp

78d2c4b

int buffer_size -> std::streamsize buffer_size

Update gzip.hpp

59b1167

int buffer_size -> std::streamsize buffer_size

Update bzip2.hpp

6eb9c62

int buffer_size -> std::streamsize buffer_size

Update symmetric.hpp

b9e1d1d

int buffer_size -> std::streamsize buffer_size

Update code_converter.hpp

cf7c922

int buffer_size -> std::streamsize buffer_size

yanikibo force-pushed the develop branch from c7844f8 to cf7c922 Compare October 2, 2015 16:10

smanders mentioned this pull request Feb 25, 2016

build boost with zlib support smanders/externpro#14

Closed

eldiener reviewed Dec 4, 2016

View reviewed changes

eldiener merged commit b8e459a into boostorg:develop Dec 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugs in file_descriptor and gzip-zlib-bzip2 filters #17

bugs in file_descriptor and gzip-zlib-bzip2 filters #17

yanikibo commented Sep 14, 2015

eldiener Dec 4, 2016

yanikibo Dec 4, 2016 •

edited

Loading

eldiener Dec 5, 2016

yanikibo Dec 5, 2016

yanikibo Dec 5, 2016

yanikibo Dec 5, 2016

eldiener Dec 5, 2016

yanikibo Dec 5, 2016

yanikibo Dec 5, 2016

eldiener Dec 4, 2016

yanikibo Dec 4, 2016 •

edited

Loading

eldiener Dec 4, 2016

yanikibo Dec 4, 2016 •

edited

Loading

eldiener commented Dec 6, 2016

eldiener commented Dec 6, 2016

bugs in file_descriptor and gzip-zlib-bzip2 filters #17

bugs in file_descriptor and gzip-zlib-bzip2 filters #17

Conversation

yanikibo commented Sep 14, 2015

Choose a reason for hiding this comment

yanikibo Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanikibo Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanikibo Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

5. deflate() or inflate() returns Z_BUF_ERROR:

eldiener commented Dec 6, 2016

eldiener commented Dec 6, 2016

yanikibo Dec 4, 2016 •

edited

Loading

yanikibo Dec 4, 2016 •

edited

Loading

yanikibo Dec 4, 2016 •

edited

Loading