[THRIFT-840] Perl protocol handler could be more robust against unrecognised types

• Type: Bug
• Status: Closed
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: 0.3
• Fix Version/s: 0.5
• Component/s: Perl - Library

Environment

Debian Linux 5.0 (stable), Perl 5.10.0.

• Patch Info: Patch Available
• Assignee: Conrad Hughes
• Reporter: Conrad Hughes
• Created: 06/Aug/10 20:46
• Updated: 02/Sep/10 00:26
• Resolved: 02/Sep/10 00:26

Description

The Perl protocol implementation can loop arbitrarily on corrupt data because Protocol::skip() skips nothing if it doesn't recognise a type. It might be nice if it threw an exception instead. For example, the loop to skip a map (lib/perl/lib/Thrift/Protocol.pm:374) will keep looking at the same invalid bytes for every iteration if it hits an unrecognised type. If corrupt data has resulted in a bogus huge map size, then the loop goes on for a long while, rather than dying quickly after running out of available data.

I recognise that input validation probably isn't a priority for Thrift (usually communications are well-formed!), but wonder if you'd consider the attached patch which dies if skip or skipBinary encounter types that they don't know what to do with. It probably needs more thought than I've given it, as I'm not sure what the correct behaviour should be for valid types not handled by the existing code: in the patch I'm throwing an exception for those saying that they cannot be skipped. An additional TType constant of MAX_TYPE might be helpful in writing a better solution.

I know it's a bit weird to be suggesting this; I'm using Thrift for serialisation among other things, and with an occasionally-imperfect data store. This "infinite" loop was the only instance in which your existing error handling didn't adequately flag up bad data. Clearly I should also be doing checksums on the data beforehand, but I just thought I'd suggest this to you.

Thanks,
Conrad