[jira] [Issue Comment Edited] (THRIFT-1182) Native deserializer segfaults on incorrect list element type

["Ilya Maykov (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13041419#comment-13041419 ] 

Ilya Maykov edited comment on THRIFT-1182 at 5/31/11 4:56 AM:
--------------------------------------------------------------

The original patch didn't call read\_(list|map|set)\_end if the contents were skipped, which is incorrect. It happens to work for Binary and Compact protocols because the read\_(list|map|set)\_methods are no-ops, but would fail with a future implementation of JSON protocol, for example.

      was (Author: ilyam):
    The original patch didn't call read_(list|map|set)_end if the contents were skipped, which is incorrect. It happens to work for Binary and Compact protocols because the read_(list|map|set)_methods are no-ops, but would fail with a future implementation of JSON protocol, for example.
  
> Native deserializer segfaults on incorrect list element type
> ------------------------------------------------------------
>
>                 Key: THRIFT-1182
>                 URL: https://issues.apache.org/jira/browse/THRIFT-1182
>             Project: Thrift
>          Issue Type: Bug
>          Components: Ruby - Library
>    Affects Versions: 0.6.1
>         Environment: OS X 10.6 i686, Linux x86_64
>            Reporter: Ilya Maykov
>         Attachments: patch-THRIFT-1182-2.txt, patch-THRIFT-1182.txt, thrift_crash.rb
>
>
> There is a pretty major bug in the native Ruby deserializer that causes it to segfault on certain bad inputs. Basically when deserializing a list that is expected to contain elements of a basic type, but is claiming in the list header to be a list of structs, the native deserializer segfaults and crashes the ruby process. I will be attaching code that reproduces this shortly.
> I need to have a fix for this ASAP, and am willing to work on it, however I need some guidance, as I'm not sure what the desired behavior is. Basically the case is:
> * Expecting a list<string> (or list<i32>, list<i16>, etc.)
> * Get something that claims to be a list<struct>
> This can be caused by a buggy and/or malicious client, or by accidentally making a backwards-incompatible change to a .thrift definition and running both versions concurrently. Regardless of the cause, crashing on arbitrary inputs is not an option for my use case so I have to handle it somehow. I see 2 possible solutions:
> 1) Raise some kind of Type Mismatch error and catch it higher up, thus aborting parsing of the entire thrift struct
> 2) Skip the list with mismatched type but attempt to parse the rest of the struct.
> Of course, if the list header is wrong about the element type it could be wrong about the list length as well so it's not clear how many bytes should be skipped. Basically once such a type error is detected, the contents of the serialized struct can no longer be trusted. For this reason, I would lean towards option 1.
> Right now it doesn't look like the deserializer does much type checking while deserializing, so such a change in behavior could break numerous existing users and should probably be optional (i.e. right now you could have a list<i64> declared but a list<i32> come across the wire, I *think* the native deserializer will just read the list of i32s and happily convert them to Ruby Fixnums so everything will work. Adding the type checks I suggest would break this case.)

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira