Boost C++ Libraries: Ticket #3723: gzip_docompressor error - basic_array_source no putback member

Nikolay Mladenov — Mon, 01 Mar 2010 02:26:14 GMT

Would you accept this patch? Or is it not general enough?

$ svn diff -r HEAD Index: detail/adapter/direct_adapter.hpp =================================================================== --- detail/adapter/direct_adapter.hpp (revision 59992) +++ detail/adapter/direct_adapter.hpp (working copy) @@ -116,6 +116,7 @@

std::streamsize write(const char_type* s, std::streamsize n); std::streampos seek( stream_offset, BOOST_IOS::seekdir,

BOOST_IOS::openmode = BOOST_IOS::in | BOOST_IOS::out );

+ bool putback(const char_type c);

void close(); void close(BOOST_IOS::openmode which);

#ifndef BOOST_IOSTREAMS_NO_LOCALE

@@ -204,6 +205,18 @@

}

template<typename Direct>

+inline bool direct_adapter<Direct>::putback + (const char_type c) +{ + pointers& get = ptrs_.first(); + if( get.ptr == get.beg ) + throw bad_putback(); + get.ptr -= 1; + *get.ptr = c; + return true; +} + +template<typename Direct>

inline std::streamsize direct_adapter<Direct>::write

(const char_type* s, std::streamsize n)

{

Nikolay Mladenov — Mon, 01 Mar 2010 02:28:47 GMT

Sorry for the mess. Reposting the patch.

$ svn diff -r HEAD
Index: detail/adapter/direct_adapter.hpp
===================================================================
--- detail/adapter/direct_adapter.hpp   (revision 59992)
+++ detail/adapter/direct_adapter.hpp   (working copy)
@@ -116,6 +116,7 @@
     std::streamsize write(const char_type* s, std::streamsize n);
     std::streampos seek( stream_offset, BOOST_IOS::seekdir,
                          BOOST_IOS::openmode = BOOST_IOS::in | BOOST_IOS::out );
+    bool putback(const char_type c);
     void close();
     void close(BOOST_IOS::openmode which);
 #ifndef BOOST_IOSTREAMS_NO_LOCALE
@@ -204,6 +205,18 @@
 }
 template<typename Direct>
+inline bool direct_adapter<Direct>::putback
+    (const char_type c)
+{
+    pointers& get = ptrs_.first();
+    if( get.ptr == get.beg )
+        throw bad_putback();
+    get.ptr -= 1;
+    *get.ptr = c;
+    return true;
+}
+
+template<typename Direct>
 inline std::streamsize direct_adapter<Direct>::write
     (const char_type* s, std::streamsize n)
 {

Daniel James — Sat, 06 Mar 2010 14:15:48 GMT

This looks wrong to me, I don't think you should be modifying the source array. IMO the correct solution is to remove the call to putback in basic_gzip_decompressor::peekable_source::putback and change it so that it either throws an error or puts the character at the start of the buffer. The attached patch does the former, looking at the gzip implementation it only ever calls putback with a string or immediately after reading a character so it should never throw. Does it look okay to you?

attachment set

Daniel James — Sat, 06 Mar 2010 14:16:51 GMT

attachment → gzip.patch

anonymous — Mon, 08 Mar 2010 18:59:47 GMT

I see, I never even tried to understand the gzip code, just assumed that it *really* needs to putback in the source.

Does it look ok to you?

Of course. As long as it fixes my issue.

But I don't see why not have the array source peekable anyway(why is it wrong)? Aren't there other filters that require peekability?

Not trying to be pushy:) ...

milestone changed

Daniel James — Mon, 08 Mar 2010 19:28:29 GMT

milestone Boost 1.42.0 → Boost 1.43.0

The problem is that the 'peekable' interface doesn't just peek at a character, it lets you put back a different character, modifying the source array, which is read only according to the documentation (it probably should take a const reference, but that's another matter). Did you test my patch with your code?

Daniel James — Mon, 08 Mar 2010 19:32:06 GMT

Sorry, bad link. Here's the correct link: http://www.boost.org/doc/libs/1_42_0/libs/iostreams/doc/classes/array.html#array_source

Daniel James — Wed, 10 Mar 2010 07:29:45 GMT

(In [60415]) Gzip filter shouldn't require its source to be peekable. Refs #3723.

In a recent version, the gzip filter stopped working for array sources, this is because it started to require them to be peekable, which they aren't and can't be because the peek interface modifies the source, which for an array source is immutable.

Looking at the implementation, gzip decompressor has an internal class to emulate a peekable source, which calls the putback member on the original source if it runs out of space (requiring the source to be peekable). It shouldn't really need to do that so I changed it to throw an exception instead.

If it does need to do that, we could change it to store the character that was put back at the beginning of the string instead.

Nikolay Mladenov — Wed, 10 Mar 2010 18:02:39 GMT

Replying to danieljames:

Did you test my patch with your code?

Yes. It works for me. Thank you!

modifying the source array, which is read only

what about basic_array?

</teasing>

owner, status changed

Daniel James — Fri, 12 Mar 2010 19:36:49 GMT

owner changed from Jonathan Turkanis to Daniel James
status new → assigned

status changed; resolution set

Daniel James — Wed, 17 Mar 2010 00:23:34 GMT

status assigned → closed
resolution → fixed

(In [60666]) Merge iostreams.

Fix write_device_impl<ostream_tag>. Fixes #3839
Fix error checks after calling SetFilePointer. Fixes #3953
Gzip filter shouldn't require its source to be peekable. Fixes #3723.
In position_to_offset, only cast to stream_offset after calculating _Myoff. Fixes #3969.
ptrdiff_t is in std. Fixes #2505.