SERVER-32191 DocumentSourceSkip can overflow a long long when combining with adjacent skips #1225
Conversation
| _nToSkip += nextSkip->getSkip(); | ||
| container->erase(std::next(itr)); | ||
| return itr; | ||
| if (std::numeric_limits<long long>::max() - _nToSkip - nextSkip->getSkip() >= 0) { |
There was a problem hiding this comment.
Please add a comment here describing that we want to avoid overflowing '_nToSkip'. This only works because we know the skips are non-negative, right? If _nToSkip were negative, it looks like we'd overflow.
| @@ -65,5 +65,28 @@ TEST_F(DocumentSourceSkipTest, ShouldPropagatePauses) { | |||
| ASSERT_TRUE(skip->getNext().isEOF()); | |||
| } | |||
|
|
|||
| TEST_F(DocumentSourceSkipTest, SkipsChainedTogetherShouldNotOverFlowWhenOptimizing) { | |||
| // $skip should not optimize if overflow | |||
There was a problem hiding this comment.
'if overflow' doesn't really make sense. How about 'if combining the two values of skips would overflow a long long'. Ditto for the comment below.
| TEST_F(DocumentSourceSkipTest, SkipsChainedTogetherShouldNotOverFlowWhenOptimizing) { | ||
| // $skip should not optimize if overflow | ||
| auto skipShort = DocumentSourceSkip::create(getExpCtx(), 1); | ||
| auto skipLong = DocumentSourceSkip::create(getExpCtx(), std::numeric_limits<long long>::max()); |
There was a problem hiding this comment.
Can you add a test to make sure that two $skips with std::numeric_limits<long long>::max() will not combine?
| // $skip should not optimize if overflow | ||
| auto skipShort = DocumentSourceSkip::create(getExpCtx(), 1); | ||
| auto skipLong = DocumentSourceSkip::create(getExpCtx(), std::numeric_limits<long long>::max()); | ||
| Pipeline::SourceContainer overFlowContainer; |
There was a problem hiding this comment.
total nitpick, but if overflow is one word, this should be 'overflowContainer' not 'overFlowContainer'.
KevinCybura
force-pushed
the
longOverFlow
branch
from
0e6b3c4
to
6365eee
Compare
| _nToSkip += nextSkip->getSkip(); | ||
| container->erase(std::next(itr)); | ||
| return itr; | ||
| // '_nToSkip' can potentially overflow causing it to become negative and skip nothing |
There was a problem hiding this comment.
All the changes look good here, thanks! Can you be sure to end all comments in periods: https://github.com/mongodb/mongo/wiki/Style-Guidelines#general-guidelines-for-commenting-code
Other than that this looks ready to go!
No description provided.