Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#605 Base64Decoder drops 0x0d characters on windows

Feature_Request
open
nobody
None
1
2012-12-21
2012-12-19
Aviad Rozenhek
No
Poco::Base64Decoder doesn't work properly on windows systems. the code below shows a unit test that fails on windows with poco 1.44 [didn't test on other releases].

the proposed fix is adding a call to this->unsetf(std::ios_base::skipws); in the Base64Decoder constructor

   #include <Poco/Base64Decoder.h>
   #include <algorithm>
   #include <iterator>
   #include <stdint.h>
   #include <gtest/gtest.h>
   TEST(poco_base64decode_tests, decode_0x0d_test)
   {
       std::istringstream input("DQ==");
       Poco::Base64Decoder decoder(input); // the fix is adding the following line: decoder.unsetf(std::ios_base::skipws);
       std::vector<uint8_t> result;
       typedef std::istream_iterator<uint8_t> istream_iterator_type;
       istream_iterator_type eos;
       std::copy(istream_iterator_type(decoder), eos, std::back_inserter(result));
       ASSERT_EQ(1, result.size()); // fails, because result.size() is 0
       ASSERT_EQ(0x0d, result[0]); // fails
   }

Discussion

  • Alex Fabijanic
    Alex Fabijanic
    2012-12-20

    • status: open --> accepted
     
  • This is standard iostreams behavior and not specific to Base64Encoder. What we now have is Base64Decoder behaving differently than all other things, which is certainly not desirable.
    Calling unsetf(std::ios_base::skipws); should be done by client code, not the stream constructor.

     
  • Alex Fabijanic
    Alex Fabijanic
    2012-12-20

    I see your point. It is, however, a very annoying standard feature. In fact, so much so, that there is a proposal for standard workaround:
    http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3431.html
    I agree that the current fix is painting with a too broad brush, but can we have either (a) default constructor bool argument (defaulting to skipws) or (b) additional constructor taking bool argument. If we reach a consensus, this could be extended to other poco streams.

     
  • Aviad Rozenhek
    Aviad Rozenhek
    2012-12-20

    without skipws, Base64Decoder fails to do Base64 decoding.
    there is no way to use Base64Decoder correctly without setting this flag.

     
  • Alex Fabijanic
    Alex Fabijanic
    2012-12-20

    Aviad, that is understood. What Günter is saying is that it is standard streams behavior, therefore user's responsibility to set it.

     
  • Aviad Rozenhek
    Aviad Rozenhek
    2012-12-20

    Alex, thank you for reviewing my bug report :-)

    as I see it, the fact that Base64Decoder is a stream at all, is rather an implementation detail of the class, that enables easy [yet wrong!] usage for decoding large blocks. see http://farid.hajji.name/blog/2010/03/21/cpp-tutorial-5/ for a very enthusiastic user of poco, that appears first when searching for Base64Decoder on google [https://www.google.com/search?q=poco+Base64decoder+sample] that got it wrong too.

    IMHO your expectation that the user will set the skipws flag is unreasonable. being a seasoned developer, I've spent 4 hours until I found how to get Base64Decoder to work properly.
    there is no value in NOT setting skipws on Base64Decoder because it causes the class to incorrectly perform it's duties.

     
  • The actual problem you have is using formatted I/O on binary data. istream_iterator, even for [signed|unsigned] char is still formatted I/O, and as a result will skip whitespace, if you don't specify otherwise. You'll get the same result if you read from a binary file. The issue does not happen if you use unformatted input functions like read(char* s, streamsize n). Unfortunately, this is a common mistake people make with iostreams. Adding a proper comment to the class documentation may make more sense than changing the standard behavior of a stream. Furthermore, Base64Decoder is not the only class affected by this behavior. We'd have to do this for all similar classes, probably all stream classes in POCO. But as I said, I don't like the idea of POCO's stream classes having a different behavior than std stream classes.

     
    Last edit: Guenter Obiltschnig 2012-12-20
  • Aviad Rozenhek
    Aviad Rozenhek
    2012-12-20

    its not neither clear nor very convenient to properly use read(char* s, streamsize n) because its unclear how many bytes are available in the stream.

    again, let me stress that the highest ranking link in the net that shows a sample of using this class doesn't work properly because of this issue.

    I'm quite sure that if you do try to provide non-trivial usage sample and/or unit test of the class with the library, you would see that the API of the class is quite misleading.

     
  • Alex Fabijanic
    Alex Fabijanic
    2012-12-20

    With poco streams being an extension of the standard IO stream inheritance tree, this is fundamentally the standard problem. We are, however, "guilty by association" and we should know better (as Meyers said, "interfaces should be easy to use correctly and hard to use incorrectly"). So, I would still be inclined to consistently offer some options at construction, at least for streams dealing primarily with binary data. I will revert this change in github, not include it in 1.5.1, create a separate github branch and we can see where to take it from there.

     
  • Alex Fabijanic
    Alex Fabijanic
    2012-12-21

    • status: accepted --> open
    • milestone: Platform_Specific --> Not_a_Bug
     
  • Alex Fabijanic
    Alex Fabijanic
    2012-12-21

    • milestone: Not_a_Bug --> Feature_Request