Huaisi Xu has posted comments on this change. Change subject: IMPALA-3494: Fix Thrift TMemoryBuffer overflow ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/3149/1/source/thrift/thrift-0.9.0-patches/0005-IMPALA-3494-fix-Thrift-TMemoryBuffer-overflow.patch File source/thrift/thrift-0.9.0-patches/0005-IMPALA-3494-fix-Thrift-TMemoryBuffer-overflow.patch: Line 9: > > Shouldn't it be >=? I this we can still shift 0x7fffffff one place to the left without overflow. but not 0x7fffffff+1. Line 10: TMemoryBuffer reached 2 GB, unable to expand > That error message is not accurate because buffer_ hasn't increased yet. Th no, this function will be called frequently in a single serialization. the buffer can grow from kb to gb. it is not one time call. if thrift write a object from 10k objects when this buffer was already expanded to 2gb-1 then it will trigger this. but to report the length of this object is not informative I think.. Line 15: > nit: remove extra space This is generated by git diff. other diffs also have this. But this wont get in code after apply this diff I think. Line 22: bufferSize_ = new_size; : : - ptrdiff_t offset = (uint8_t*)new_buffer - buffer_; : - buffer_ += offset; : - rBase_ += offset; : - rBound_ += offset; : - wBase_ += offset; : - wBound_ = buffer_ + bufferSize_; > Why did you change that? I think this prevent some memory alignment issue. https://github.com/apache/thrift/commit/6077481139933b927397c7da0088aa4678f9fb3c so just grab that.. if you do not like I can just remove this.. -- To view, visit http://gerrit.cloudera.org:8080/3149 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I91086ed9d0b4f95b5532de67fe35b46b748ad16c Gerrit-PatchSet: 1 Gerrit-Project: Toolchain Gerrit-Branch: master Gerrit-Owner: Huaisi Xu <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Huaisi Xu <[email protected]> Gerrit-HasComments: Yes
