Track the amount of read data per row #2368
Conversation
There was a problem hiding this comment.
Do you want to use TypeSizes instead here?
There was a problem hiding this comment.
well, I don't think so, that would require also refactoring that in bytesRead updates
There was a problem hiding this comment.
I think it is more clear to use the size number 1/2/4/8 ,which make the code more readable as the code below just add these numbers
There was a problem hiding this comment.
Not following. Why would you need to refactor anything? You guys think 1 is more readable than TypeSizes.BOOL_SIZE and on top of that you're hardcoding a value? It's like magic numbers where we have a proper abstraction ready unless I am missing sthg.
checkCanRead(TypeSizes.BOOL_SIZE) reads good in my eyes 🤷
There was a problem hiding this comment.
I agree with your point of view. From the perspective of code readability, TypeSizes.BOOL_SIZE is definitely much better than hard code 1.
What I said above means that if the existing code is already written as 1/4/8, our newly added code also fills in 1/4/8, then it just echoes back and forth, so the readability is not bad.
Of course, if Jacek is willing to change the existing numbers to TypeSizes, I definitely support your point of view.
There was a problem hiding this comment.
(1)checkCanRead(TypeSizes.BOOL_SIZE); byte b = source.readByte(); bytesRead += TypeSizes.BOOL_SIZE; return b;
(2) checkCanRead(1); byte b = source.readByte(); bytesRead += 1; return b;
(3)checkCanRead(TypeSizes.BOOL_SIZE); byte b = source.readByte(); bytesRead += 1; return b;
I think the code itself should explain its behavior.
just from my point of view, this order is (1) > (2) > (3)
and it seems Jacek has already resolved this issue.
|
We need CI runs and will you port this to 4.0 and 4.1 also? |
|
@bereng thanks for super quick response. I'll provide patches for older branches but probably it will be next week |
There was a problem hiding this comment.
will limit just be zero？
There was a problem hiding this comment.
I just test some times,it seems limit can not be 0 ~~~
There was a problem hiding this comment.
Yes, we always encode at least a flag
There was a problem hiding this comment.
I think a test for some corner case is needed such as what I said when limit is 0？
|
@bereng is it ok now? |
|
Approach looks good to me. CI and PRs for the rest of the versions are missing iiuc. |
|
@bereng there was no point in running CI before code review, same for other PRs |
|
Ha you're brave. I always run CI before... |
If an sstable is corrupted in a nasty way, we may read invalid cell sizes and try to read much more data for a row than we should. In rare scenarios this can lead even to OOMs. This simple fix adds tracking and limiting the amount of data that is read per row. Row has its size stored in preamble which can be used as a limit. If the deserialization code tries to read more than that, it will simply fail with EOF which will prevent more serious problems later. Patch by Jacek Lewandowski; reviewed by Berenguer Blasi and Maxwell Guo for CASSANDRA-18513
jacek-lewandowski
force-pushed
the
CASSANDRA-18513
branch
from
7400fc5 to
02b188a
Compare
|
^That is j11 only, j8 seems to not have been triggered? |
belliottsmith
force-pushed
the
trunk
branch
2 times, most recently
from
df3eb40 to
54e39a9
Compare
3 participants
If an sstable is corrupted in a nasty way, we may read invalid cell sizes and try to read much more data for a row than we should. In rare scenarios this can lead even to OOMs.
This simple fix adds tracking and limiting the amount of data that is read per row. Row has its size stored in preamble which can be used as a limit. If the deserialization code tries to read more than that, it will simply fail with EOF which will prevent more serious problems later.
Thanks for sending a pull request! Here are some tips if you're new here:
Commit messages should follow the following format:
The Cassandra Jira