#4201 binary decode uuencode foo --> segfault

obsolete: 8.6a4
closed-fixed
Pat Thoyts
9
2008-12-15
2008-12-03
Duoas
No

Yep, trying to decode an invalid string causes a segfault (meaning Tcl crashes and burns).

To replicate, start up tclsh86 and type:

binary decode uuencode foo

Hope this helps.

Discussion

    • milestone: --> obsolete: 8.6a4
    • priority: 5 --> 9
    • assigned_to: dkf --> patthoyts
     
  • Saturate buggy nonstrict size

     
    Attachments
  • This is due to an off-by-one length computation yielding -1 in a non-strict case.
    Knowing nothing of the uuencode algorithm I cannot fix it properly; however, the attached patch fixes the crash by saturating the size to nonnegative values.
    File Added: uu.patch

     
  • OK now I know the algorithm. The off-by-one is the visible part of a larger iceberg: the "cut" variable counts missing bytes in the source (base 64) stream, and is wrongly translated 1-to-1 into missing bytes in the decoded stream. So there's a systematic error of factor 4/3 (since a "cut++" should only count as removing 6 bits).

    However, I believe the very principle of allowing non-strict uudecoding is flawed. Indeed, a "BadUU" byte in the middle is just skipped, but I wonder about the perturbation process that this is modelling. Indeed if random bytes are inserted here and there, only those which are fortunately outside the 32..96 range will be filtered out. So I cannot imagine a practical situation where an uuencoded stream would be decoded non-strictly... So the quick patch is enough to avoid crashes, the only remaining error being truncated results for error-filled inputs.

    Pat, your feeling ?

     
  • After a chat with Kevin, only answerer, it appears that non-strictness has an obvious usecase with randomly interspersed whitespace. That was the perturbation process I was after.
    But it is also clear that any other kind of unexpected character is fishy and should raise an error.
    Just committed this change:
    - fix the off-by-one bug that caused crashes or truncation (which is not 4/3 but only of one char at maximum, because it can occur only in the last 4-byte chunk)
    - redefine the non-strict contract to allow only whitespace as defined by the isspace() function.

     
    • status: open --> closed-fixed