You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Conrad Hughes (JIRA)" <ji...@apache.org> on 2010/08/06 22:47:18 UTC
[jira] Created: (THRIFT-840) Perl protocol handler could be more
robust against unrecognised types
Perl protocol handler could be more robust against unrecognised types
---------------------------------------------------------------------
Key: THRIFT-840
URL: https://issues.apache.org/jira/browse/THRIFT-840
Project: Thrift
Issue Type: Bug
Components: Library (Perl)
Affects Versions: 0.3
Environment: Debian Linux 5.0 (stable), Perl 5.10.0.
Reporter: Conrad Hughes
Priority: Minor
Fix For: 0.3
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
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-840) Perl protocol handler could be more
robust against unrecognised types
Posted by "Conrad Hughes (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-840?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Conrad Hughes updated THRIFT-840:
---------------------------------
Attachment: perl_unrecognised_types.patch
Throw exception instead of silent return on unrecognised type.
> Perl protocol handler could be more robust against unrecognised types
> ---------------------------------------------------------------------
>
> Key: THRIFT-840
> URL: https://issues.apache.org/jira/browse/THRIFT-840
> Project: Thrift
> Issue Type: Bug
> Components: Library (Perl)
> Affects Versions: 0.3
> Environment: Debian Linux 5.0 (stable), Perl 5.10.0.
> Reporter: Conrad Hughes
> Priority: Minor
> Fix For: 0.3
>
> Attachments: perl_unrecognised_types.patch
>
>
> 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
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (THRIFT-840) Perl protocol handler could be more
robust against unrecognised types
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-840?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury closed THRIFT-840.
--------------------------------
Assignee: Conrad Hughes
Resolution: Fixed
I committed this with the redundant parts removed. Thanks for the patch!
> Perl protocol handler could be more robust against unrecognised types
> ---------------------------------------------------------------------
>
> Key: THRIFT-840
> URL: https://issues.apache.org/jira/browse/THRIFT-840
> Project: Thrift
> Issue Type: Bug
> Components: Perl - Library
> Affects Versions: 0.3
> Environment: Debian Linux 5.0 (stable), Perl 5.10.0.
> Reporter: Conrad Hughes
> Assignee: Conrad Hughes
> Priority: Minor
> Fix For: 0.5
>
> Attachments: perl_unrecognised_types.patch
>
>
> 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
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-840) Perl protocol handler could be more
robust against unrecognised types
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-840?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-840:
---------------------------------
Fix Version/s: 0.5
(was: 0.4)
> Perl protocol handler could be more robust against unrecognised types
> ---------------------------------------------------------------------
>
> Key: THRIFT-840
> URL: https://issues.apache.org/jira/browse/THRIFT-840
> Project: Thrift
> Issue Type: Bug
> Components: Library (Perl)
> Affects Versions: 0.3
> Environment: Debian Linux 5.0 (stable), Perl 5.10.0.
> Reporter: Conrad Hughes
> Priority: Minor
> Fix For: 0.5
>
> Attachments: perl_unrecognised_types.patch
>
>
> 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
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-840) Perl protocol handler could be more
robust against unrecognised types
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-840?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-840:
---------------------------------
Fix Version/s: 0.4
(was: 0.3)
> Perl protocol handler could be more robust against unrecognised types
> ---------------------------------------------------------------------
>
> Key: THRIFT-840
> URL: https://issues.apache.org/jira/browse/THRIFT-840
> Project: Thrift
> Issue Type: Bug
> Components: Library (Perl)
> Affects Versions: 0.3
> Environment: Debian Linux 5.0 (stable), Perl 5.10.0.
> Reporter: Conrad Hughes
> Priority: Minor
> Fix For: 0.4
>
> Attachments: perl_unrecognised_types.patch
>
>
> 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
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-840) Perl protocol handler could be more
robust against unrecognised types
Posted by "Conrad Hughes (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-840?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12898024#action_12898024 ]
Conrad Hughes commented on THRIFT-840:
--------------------------------------
No, you're exactly right. I separated them out because I thought it
might be informative in case it turns out that all those valid types for
which there's no skip code should just return 0. I don't know what
should be done for them really, but looking at the Java skip code they
aren't handled there either. It appears to me that you can only reach
the end of that method in error, so any exception will do as far as I'm
concerned.
Conrad
--
The University of Edinburgh is a charitable body, registered in
Scotland, with registration number SC005336.
> Perl protocol handler could be more robust against unrecognised types
> ---------------------------------------------------------------------
>
> Key: THRIFT-840
> URL: https://issues.apache.org/jira/browse/THRIFT-840
> Project: Thrift
> Issue Type: Bug
> Components: Library (Perl)
> Affects Versions: 0.3
> Environment: Debian Linux 5.0 (stable), Perl 5.10.0.
> Reporter: Conrad Hughes
> Priority: Minor
> Fix For: 0.5
>
> Attachments: perl_unrecognised_types.patch
>
>
> 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
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-840) Perl protocol handler could be more
robust against unrecognised types
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-840?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12897969#action_12897969 ]
Bryan Duxbury commented on THRIFT-840:
--------------------------------------
I'm no Perl guy, but it seems like there's no reason to have two different kinds of "die" cases. The one with the if seems to be subsumed by the one without - am I wrong?
> Perl protocol handler could be more robust against unrecognised types
> ---------------------------------------------------------------------
>
> Key: THRIFT-840
> URL: https://issues.apache.org/jira/browse/THRIFT-840
> Project: Thrift
> Issue Type: Bug
> Components: Library (Perl)
> Affects Versions: 0.3
> Environment: Debian Linux 5.0 (stable), Perl 5.10.0.
> Reporter: Conrad Hughes
> Priority: Minor
> Fix For: 0.4
>
> Attachments: perl_unrecognised_types.patch
>
>
> 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
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.