You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Ilya Maykov (JIRA)" <ji...@apache.org> on 2011/05/29 04:13:47 UTC
[jira] [Created] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
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
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
[jira] [Closed] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury closed THRIFT-1182.
---------------------------------
Resolution: Fixed
Fix Version/s: 0.7
Assignee: Ilya Maykov
I just committed this patch. Thanks, Ilya!
> 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
> Assignee: Ilya Maykov
> Fix For: 0.7
>
> 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
[jira] [Commented] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Ilya Maykov (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13040743#comment-13040743 ]
Ilya Maykov commented on THRIFT-1182:
-------------------------------------
Per IRC chat with Bryan Duxbury, the deserializer should skip over list fields (presumably also sets, maps) with unexpected element types, but should keep trying to parse the rest of the struct.
> 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: 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
[jira] [Updated] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Ilya Maykov (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ilya Maykov updated THRIFT-1182:
--------------------------------
Attachment: patch-THRIFT-1182.txt
Wrong file attached last time, here goes try 2.
> 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.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
[jira] [Commented] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Hudson (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13041820#comment-13041820 ]
Hudson commented on THRIFT-1182:
--------------------------------
Integrated in Thrift #150 (See [https://builds.apache.org/hudson/job/Thrift/150/])
THRIFT-1182. rb: Native deserializer segfaults on incorrect list element type
This patch causes both the pure ruby and native extension code paths to check if the data in lists, sets, and maps is of the expected type before deserlizing it. When it's not the right type, it now skips the bad data correctly.
Patch: Ilya Maykov
bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1129892
Files :
* /thrift/trunk/lib/rb/lib/thrift/struct_union.rb
* /thrift/trunk/lib/rb/ext/struct.c
> 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
> Assignee: Ilya Maykov
> Fix For: 0.7
>
> 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
[jira] [Updated] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Ilya Maykov (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ilya Maykov updated THRIFT-1182:
--------------------------------
Attachment: (was: patch-THRIFT-1182.txt)
> 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: 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
[jira] [Updated] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Ilya Maykov (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ilya Maykov updated THRIFT-1182:
--------------------------------
Patch Info: [Patch Available]
> 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.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
[jira] [Updated] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Ilya Maykov (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ilya Maykov updated THRIFT-1182:
--------------------------------
Attachment: patch-THRIFT-1182.txt
Attaching patch.
With this patch, the deserializer will skip any maps / lists / sets whose key/value/element types do not match the expected key/value/element types for that field.
Tested with all combinations of:
protocols: BinaryProtocol, BinaryProtocolAccelerated, CompactProtocol
rubies: 1.8.7, 1.9.2
native extensions: enabled, disabled
> 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.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
[jira] [Updated] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Ilya Maykov (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ilya Maykov updated THRIFT-1182:
--------------------------------
Attachment: patch-THRIFT-1182-2.txt
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
[jira] [Issue Comment Edited] (THRIFT-1182) Native deserializer
segfaults on incorrect list element type
Posted by "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
[jira] [Updated] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Ilya Maykov (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ilya Maykov updated THRIFT-1182:
--------------------------------
Attachment: thrift_crash.rb
Attached simple repro. Run with thrift 0.6.0 gem, tested on ruby 1.8.7 and 1.9.2.
> 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: 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
[jira] [Commented] (THRIFT-1182) Native deserializer segfaults on
incorrect list element type
Posted by "Ilya Maykov (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1182?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13040764#comment-13040764 ]
Ilya Maykov commented on THRIFT-1182:
-------------------------------------
Sorry the patch is against a copy of the source that i've mirrored on our own git repo, so the file paths may look funky.
> 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.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