Hi list!

The Base64 decoding stream in the Base64 unit in the FCL appears to be broken. In particular
- it does not handle whitespace (or other characters not from the bas64 alphabet), and
- Read does not return the correct number of bytes read when at the end of the stream, and
- the meaning of the EOF is a little bit unclear

These bugs can be circumvented by filtering out whitespace first and calling Size [which works!] to determine the actual stream size, but this is of course ugly, slower and not working on non-seekable streams.

I saw there already was a small fix after 2.0.4, but there is going on quite some more.

Note that the source code mentions RFC2045, which is actually about MIME, but also mentions a Base64 Content Transfer Encoding, but that RFC3548 is a preferred source, as it deals with base64 encoding in general. (URLs for RFCs: http://www.ietf.org/rfc/rfc3548.txt, http://www.ietf.org/rfc/rfc2045.txt.)

I'd be happy to send a patch for the TBase64Decoding class:
- instead of supporting certain 'magical' Seek operations used in the implementation of TSream.GetSize, override GetSize
- ignore characters not in base64 alphabet and not being '='
- try to fix EOF: it should true iff all bytes have been Read

Any comments or suggestions are welcome!

I also recommend some tests are added to test the base64 decode class. It seems to be rather untested or unreviewed at the moment (no offense!), as I saw e.g. (at line 215)

              for j := i to 3 do
                ReadBuf[i] := ReadBuf[i + 1];

which obviously must be not what the author meant.

I haven't looked at the base64 encoding class in depth; it might have problems too.

Regards,

Bram

It was not designed to do this.

The non-seekable streams should remain supported.

Feel free to do so, but keep in mind the non-seekable streams issue.

If you test it, please try adding tests in fpcunit format.

I haven't looked at the base64 encoding class in depth; it might have problems too.

Hopefully not :-)

Michael.

Hi Michael,

Thanks for your reply!

I have done a little more research; details are below.

There is code trying to deal with bytes decoded as 99 (a sentinel value), but it is not really functional.

Looking more detailed at section 2.3 of RFC3548 (http://www.ietf.org/rfc/rfc3548.txt), it is indeed stated that

'Implementations MUST reject the encoding if it contains characters outside the base alphabet when interpreting base encoded data, unless the specification referring to this document explicitly states otherwise. Such specifications may, as MIME does, instead state that characters outside the base encoding alphabet should simply be ignored when interpreting data ("be liberal in what you accept"). Note that this means that any CRLF constitute "non alphabet characters" and are ignored.'

So the current design is not wrong (from the perspective of RFC3548 alone); only in the current implementation it not actually rejects these chars (e.g. it does not raise an exception).

RFC2045, about MIME, mentioned at a comment at the top in base64.pp, states however that these chars must be ignored instead of rejected. I recommend to support this 'mode' as well, as it allows line breaks, which is quite useful in textual environments.

The point is that it currently only supports seekable streams (when calling GetSize). For Read, it doesn't need seeking of course, and this will remain so.

There is not only choice in ignoring/rejecting non-base alphabet characters, but also in dealing with '=' pad characters that are not at the end of the string. (Up to two of such '=' pad characters have to be used at the end to complete the last 4-byte sequence; this last sequence will encode 3 - nr. of '='s at the end bytes instead of 3. (Normally, in Base64 encoding, sequences of 4 bytes are used to encode 3 bytes from the input.))

In particular, the current 'getsize' calculation of TBase64DecodeStream (NB this is done in the Seek method) seeks to the end of the input stream, reads the last two characters, and determines from whether these are '=' or '==' by how much the 'div 4 * 3' calculated stream size should be decreased. If we signal an EOF at the first occurence of a '=' (besides the end of the input stream), this calculation can go very wrong. The best behavior would be to raise an exception here I think (when following RFC3548.

If I understand it correctly, input strings should also always have a byte length that is a multiple of 4.

I propose to let TBase64DecodeStream have two 'modes':
- 'strict mode': follows RFC3548, rejects any characters outside of base64 alphabet, only accepts up to two '=' characters at the end, and requires the input to have a Size being a multiple of 4; otherwise raises an EBase64DecodeException
- 'MIME mode': follows RFC2045, ignores any characters outside of base64 alphabet, takes any '=' as end of string, handles apparently truncated input streams gracefully

Also, I'd tend to make MIME mode the default, but I leave that up to you (core devs) to decide.

In code:

  TBase64DecodeMode = (bdmStrict, bdmMIME);
  ...
  TBase64DecodeStream = class(TSteam)
    ...
    property Mode:TBase64DecodeMode ...

Note that in MIME mode, GetSize will Read the entire stream, whereas Strict mode allows the calculation currently done in Seek.

If you test it, please try adding tests in fpcunit format.

What would be a good starting point to learn about how to use fpcunit? (I haven't used it before)

Regards,

Bram

Maybe you can add a property 'StrictRFC' or so ?

I also think an exception is best.

OK, ignore my proposal for the property earlier. I didn't see this :-)

No, that's fine, as it'll probably be the most used mode

Excellent !

The sources and examples :-)

I once wrote an article about it (for beginners) if you want I can send it to you.

Michael.

In particular the fcl/fpcunit/exampletests/moneytest.pp testing the money.pp example classes is a translation of the original JUnit examples from Kent Beck and Erich Gamma, found in the original paper about unit testing, JUnit Test Infected: Programmers Love Writing Tests : http://junit.sourceforge.net/doc/testinfected/testing.htm

And if you have particular questions you can ask :)

I once wrote an article about it (for beginners) if you want I can send it to you.

It's still here: http://www.freepascal.org/docs-html/fpcunit.pdf

Dean

Thank you for all the information, Dean, this is an excellent starting point to unit testing and FPCUnit.

Regards,

Bram

Hi!

I've implemented Strict and MIME mode for TBase64DecodingStream as promised. I also created some FPCUnit tests. The following motivates the meaning of TBase64DecodingStream.Size and TBase64DecodingStream.Position; please comment on whether this is the desired behavior of a 'filtering' stream.

TBase64DecodingStream acts as a filter stream on another stream, called InputStream. It starts , e.g. it does not set InputStream.Position to 0. This is important so it can decode non-seekable streams as well.

In Strict mode, it will continue decoding until the end of the InputStream; Read will raise an exceptions if any non-base64 character is encountered or when a '=' character is found before the end of the InputStream.

In MIME mode, it will ignore any non-base64 characters, and consider the first '=', or InputStream end, to mark the end of the stream.

Note: Whenever InputStream.Read returns 0, this is considered as a sign that we reached the end of the InputStream; InputStream.GetSize is not used for detecting this.

TBase64DecodingStream.GetPosition returns the number of decoded & Read bytes since the last call to Reset (or Create, which calls Reset). Setting the Position (seeking) is not supported, though this could be implemented.

Now TBase64DecodingStream.GetSize: it returns the number of (expected) decoded bytes since the last call to Reset (or Create) until the end of the Base64 encoded data in the InputStream, as implied by the current Mode. The method will always Seek the InputStream, either to go to the end of the InputStream (Strict mode), or to find the first '=' (MIME mode).

Note: changing the position or content of InputStream while using TBase64DecodingStream is operating on it will probably give very unexpected results.

Regards,

Bram

Hi,

Attached is a patched base64 unit and a unit with FPCUnit tests. I zipped it to save space; I think my previous attempt has been rejected because the message was too big.

Changes:
- Added support for a MIME and a Strict mode for TBase64DecodingStream

Regards,

Bram

You can always create a bug report being a feature request and attach the patch. Then it doesn't get lost even ;)

Bram Kuijvenhoven pravi:

A very nice and simple way of making testcases with different input parameters!
Ciao,
Dean

Thank you. Testing seems to be an excellent way to improve code quality -- I found some bugs I wouldn't have found simply by resulting errors in my application that uses the unit, and creating testcases helps thinking about potential weaknesses and circumstances your code has to deal with.

Regards,

Bram