-
Notifications
You must be signed in to change notification settings - Fork 472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ORC-589: [C++] add checks about negative dictionary entry lengths #468
Conversation
The library used to have a check for negative dictionary entry lengths. However we don't have it on current master. It causes a problem because it will use a negative value (casted to unsigned) to allocate a DataBuffer. This commit adds the missing check. Testing * added a corrupt orc file and a unit test
c++/src/ColumnReader.cc
Outdated
@@ -581,6 +581,8 @@ namespace orc { | |||
lengthDecoder->next(lengthArray + 1, dictSize, nullptr); | |||
lengthArray[0] = 0; | |||
for(uint32_t i = 1; i < dictSize + 1; ++i) { | |||
if (lengthArray[i] < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add curly brace here even for a single line if-statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
c++/test/TestCorruptFiles.cc
Outdated
namespace orc { | ||
|
||
TEST(TestCorruptFiles, testNegativeDictEntryLengths) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
c++/test/TestCorruptFiles.cc
Outdated
|
||
namespace orc { | ||
|
||
TEST(TestCorruptFiles, testNegativeDictEntryLengths) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think moving this into TestFileScan.testErrorHandling in TestFileScan.cc is simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
The library used to have a check for negative dictionary entry lengths. However we don't have it on current master. It causes a problem because it will use a negative value (casted to unsigned) to allocate a DataBuffer. This commit adds the missing check. This fixes apache#468
The library used to have a check for negative dictionary entry lengths.
However we don't have it on current master. It causes a problem because
it will use a negative value (casted to unsigned) to allocate a
DataBuffer. This commit adds the missing check.
Testing