Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1061053002:
Fix PartialCircularBuffer OOB memcpy(). (Closed)
DescriptionFix PartialCircularBuffer OOB memcpy().
BUG=474029
TEST=PartialCircularBufferTest.WrapTwiceWithSingleWrite
NOPRESUBMIT=true
Committed: https://crrev.com/ab42594ec3070953971f43b384f38f3c2b65d4ea
Cr-Commit-Position: refs/heads/master@{#326341}
Patch Set 1 #Patch Set 2 : CHECK to DCHECK #
Total comments: 10
Patch Set 3 : Addressed comments #
Total comments: 7
Patch Set 4 : Addressed comments #
Messages
Total messages: 34 (6 generated)
gzobqq@gmail.com changed reviewers: + thakis@chromium.org - grunell@chromium.org
Nico, PTAL
https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... File chrome/common/partial_circular_buffer.cc (right): https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer.cc:64: uint32 PartialCircularBuffer::Read(void* buffer, uint32 buffer_size) { Doesn't this have the same problem? https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer.cc:142: const uint8* input = static_cast<const uint8*>(buffer); I can't see the bug, but if writing a buffer several times the size of the circular buffer makes sense in some situations, should the implementation be less wasteful? This does a memcpy() per overlap at the moment. It's probably possible to do something like if (buffer_size > size_of_cycle) { buffer = buffer + buffer_size - size_of_cycle; int slop = buffer_size % size_of_cycle; position_ = position_ + (position_ - wrap_position + slop + size_of_cycle) % size_of_cycle; buffer_size = size_of_cycle; } somewhere at the top of the function and keep the exisiting code, so that every element is written only once. (warning: code written without testing or even drawing boxes on a piece of paper; it's likely wrong) https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... File chrome/common/partial_circular_buffer_unittest.cc (right): https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer_unittest.cc:194: size_t input_size = sizeof(kInputData); const size_t kInputSize https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer_unittest.cc:195: size_t large_size = input_size * 7; const size_t kLargeSize https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer_unittest.cc:196: scoped_ptr<uint8[]> large_input(new uint8[large_size]); Shouldn't this easily fit on the stack?
(+ author and reviewer; maybe they remember anything about this)
PTAL https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... File chrome/common/partial_circular_buffer.cc (right): https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer.cc:64: uint32 PartialCircularBuffer::Read(void* buffer, uint32 buffer_size) { On 2015/04/07 16:42:16, Nico wrote: > Doesn't this have the same problem? It is a complicated function. I spent some time thinking about it though and it is probably correct. https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer.cc:142: const uint8* input = static_cast<const uint8*>(buffer); On 2015/04/07 16:42:16, Nico wrote: > I can't see the bug The second DoWrite() lacks a bounds check and can OOB write. > but if writing a buffer several times the size of the > circular buffer makes sense in some situations, should the implementation be > less wasteful? This does a memcpy() per overlap at the moment. It's probably > possible to do something like > > if (buffer_size > size_of_cycle) { > buffer = buffer + buffer_size - size_of_cycle; > int slop = buffer_size % size_of_cycle; > position_ = position_ + (position_ - wrap_position + slop + size_of_cycle) % > size_of_cycle; > buffer_size = size_of_cycle; > } > > somewhere at the top of the function and keep the exisiting code, so that every > element is written only once. > > (warning: code written without testing or even drawing boxes on a piece of > paper; it's likely wrong) It's a bit more complicated, because the part to skip might be in the middle of the buffer. That happens when the first part is non-wrapping, position_ < wrap_position. Done. https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... File chrome/common/partial_circular_buffer_unittest.cc (right): https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer_unittest.cc:194: size_t input_size = sizeof(kInputData); On 2015/04/07 16:42:16, Nico wrote: > const size_t kInputSize Done. https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer_unittest.cc:195: size_t large_size = input_size * 7; On 2015/04/07 16:42:16, Nico wrote: > const size_t kLargeSize Done. https://codereview.chromium.org/1061053002/diff/20001/chrome/common/partial_c... chrome/common/partial_circular_buffer_unittest.cc:196: scoped_ptr<uint8[]> large_input(new uint8[large_size]); On 2015/04/07 16:42:16, Nico wrote: > Shouldn't this easily fit on the stack? Done.
This lgtm. The distribution of work between Write() and DoWrite feels a bit awkward. https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... File chrome/common/partial_circular_buffer.cc (right): https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... chrome/common/partial_circular_buffer.cc:64: uint32 PartialCircularBuffer::Read(void* buffer, uint32 buffer_size) { Ah, hmm, this doesn't allow reading more than the buffer size, I see. So if I write 100 bytes to the buffer (with your patch) and the buffer size is only 6, I'll only get 6 bytes back. That makes sense. https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... chrome/common/partial_circular_buffer.cc:163: // Finally write the wrapping part. Say that this loop will run at most twice. https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... chrome/common/partial_circular_buffer.cc:179: if (position_ >= data_size_) If this happens, it will always be ==, not >=, right? Maybe keep the DCHECK_EQ(position_, data_size_) in this if? https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... File chrome/common/partial_circular_buffer.h (right): https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... chrome/common/partial_circular_buffer.h:55: uint32 DoWrite(const uint8* input, uint32 input_size); Document what this returns, mention that it can return less than input_size if it needs to wrap.
> This lgtm. The distribution of work between Write() and DoWrite feels a bit > awkward. Thanks for the review. I'm guessing what feels awkward is that DoWrite() handles most of the write except wrapping. I moved the loop to DoWrite(). So DoWrite() now handles all the input passed to it. I think I'll wait for another 24 hours in case you have more comments and then CQ. https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... File chrome/common/partial_circular_buffer.cc (right): https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... chrome/common/partial_circular_buffer.cc:163: // Finally write the wrapping part. On 2015/04/08 23:13:05, Nico wrote: > Say that this loop will run at most twice. Done. https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... chrome/common/partial_circular_buffer.cc:179: if (position_ >= data_size_) On 2015/04/08 23:13:05, Nico wrote: > If this happens, it will always be ==, not >=, right? Maybe keep the > DCHECK_EQ(position_, data_size_) in this if? Done. https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... File chrome/common/partial_circular_buffer.h (right): https://codereview.chromium.org/1061053002/diff/40001/chrome/common/partial_c... chrome/common/partial_circular_buffer.h:55: uint32 DoWrite(const uint8* input, uint32 input_size); On 2015/04/08 23:13:05, Nico wrote: > Document what this returns, mention that it can return less than input_size if > it needs to wrap. I changed DoWrite() to handle the complete input instead.
Nico: I guess I can't CQ before you LGTM the latest commit. PTAL once you get back.
gzobqq@gmail.com changed reviewers: + sky@chromium.org
sky@, perhaps you could take a look at this? Patch set 3 was LGTM'd by Nico. Patch set 4 fixes some nits and refactors a loop from Write() to DoWrite(). I think it needs an LGTM as well, before I can CQ this. Nico has been OOO for a while though.
On 2015/04/22 04:43:37, gzobqq wrote: > sky@, perhaps you could take a look at this? > > Patch set 3 was LGTM'd by Nico. Patch set 4 fixes some nits and refactors a loop > from Write() to DoWrite(). I think it needs an LGTM as well, before I can CQ > this. Nico has been OOO for a while though. Oh sorry I didn't see that you need another lgtm to CQ (I guess that's new?) Lgtm
gzobqq@gmail.com changed reviewers: - sky@chromium.org
On 2015/04/22 04:48:59, Nico (OOO sick) wrote: > On 2015/04/22 04:43:37, gzobqq wrote: > > sky@, perhaps you could take a look at this? > > > > Patch set 3 was LGTM'd by Nico. Patch set 4 fixes some nits and refactors a > loop > > from Write() to DoWrite(). I think it needs an LGTM as well, before I can CQ > > this. Nico has been OOO for a while though. > > Oh sorry I didn't see that you need another lgtm to CQ (I guess that's new?) > Lgtm Nico: Thanks for taking another look. The CQ box is still disabled for me. Not sure what's up with that. Perhaps because I'm an external reporter with a gmail account? Anyhow, inferno: Will the security guys take it from here for the commit and merge?
The CQ bit was checked by inferno@chromium.org
On 2015/04/22 08:55:27, gzobqq wrote: > On 2015/04/22 04:48:59, Nico (OOO sick) wrote: > > On 2015/04/22 04:43:37, gzobqq wrote: > > > sky@, perhaps you could take a look at this? > > > > > > Patch set 3 was LGTM'd by Nico. Patch set 4 fixes some nits and refactors a > > loop > > > from Write() to DoWrite(). I think it needs an LGTM as well, before I can CQ > > > this. Nico has been OOO for a while though. > > > > Oh sorry I didn't see that you need another lgtm to CQ (I guess that's new?) > > Lgtm > > Nico: Thanks for taking another look. The CQ box is still disabled for me. Not > sure what's up with that. Perhaps because I'm an external reporter with a gmail > account? Anyhow, > > inferno: Will the security guys take it from here for the commit and merge? It was a protected issue, that is why. Now i removed it and marked for cq. thanks for noticing.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1061053002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by inferno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1061053002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ab42594ec3070953971f43b384f38f3c2b65d4ea Cr-Commit-Position: refs/heads/master@{#326341}
Message was sent while issue was closed.
inferno: Thanks for explaining. Now, the trybot wants my entry in AUTHORS. I'd rather stay anonymous. I could add for example "Gzob Qq <gzobqq@gmail.com>", but it's not my real name. Would that be ok?
Message was sent while issue was closed.
On 2015/04/22 18:48:30, gzobqq wrote: > inferno: Thanks for explaining. > > Now, the trybot wants my entry in AUTHORS. I'd rather stay anonymous. I could > add for example "Gzob Qq <mailto:gzobqq@gmail.com>", but it's not my real name. Would > that be ok? Ah nevermind, CQ worked anyway.
Message was sent while issue was closed.
On 2015/04/22 18:51:18, gzobqq wrote: > On 2015/04/22 18:48:30, gzobqq wrote: > > inferno: Thanks for explaining. > > > > Now, the trybot wants my entry in AUTHORS. I'd rather stay anonymous. I could > > add for example "Gzob Qq <mailto:gzobqq@gmail.com>", but it's not my real > name. Would > > that be ok? > > Ah nevermind, CQ worked anyway. Oh, that's a great point actually. Did you sign the CLA? See https://www.chromium.org/developers/contributing-code/external-contributor-ch... I think that requires a real name, and without this we can't take your patch. (Sorry, should've checked that in addition to the code)
Message was sent while issue was closed.
On 2015/04/22 18:52:51, Nico (OOO sick) wrote: > On 2015/04/22 18:51:18, gzobqq wrote: > > On 2015/04/22 18:48:30, gzobqq wrote: > > > inferno: Thanks for explaining. > > > > > > Now, the trybot wants my entry in AUTHORS. I'd rather stay anonymous. I > could > > > add for example "Gzob Qq <mailto:gzobqq@gmail.com>", but it's not my real > > name. Would > > > that be ok? > > > > Ah nevermind, CQ worked anyway. > > Oh, that's a great point actually. Did you sign the CLA? See > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > I think that requires a real name, and without this we can't take your patch. > (Sorry, should've checked that in addition to the code) Yeah, sorry for not bringing it up before. I did sign the CLA with my real name. I'd rather not add it to the public AUTHORS though.
Message was sent while issue was closed.
On 2015/04/23 03:33:47, gzobqq wrote: > On 2015/04/22 18:52:51, Nico (OOO sick) wrote: > > On 2015/04/22 18:51:18, gzobqq wrote: > > > On 2015/04/22 18:48:30, gzobqq wrote: > > > > inferno: Thanks for explaining. > > > > > > > > Now, the trybot wants my entry in AUTHORS. I'd rather stay anonymous. I > > could > > > > add for example "Gzob Qq <mailto:gzobqq@gmail.com>", but it's not my real > > > name. Would > > > > that be ok? > > > > > > Ah nevermind, CQ worked anyway. > > > > Oh, that's a great point actually. Did you sign the CLA? See > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > I think that requires a real name, and without this we can't take your patch. > > (Sorry, should've checked that in addition to the code) > > Yeah, sorry for not bringing it up before. I did sign the CLA with my real name. > I'd rather not add it to the public AUTHORS though. If you signed the CLA, I think it's ok to add some pseudonym to the authors file as long as you also add the same email address that you put in the CLA – would that work for you?
Message was sent while issue was closed.
On 2015/04/23 03:40:58, Nico (OOO sick) wrote: > On 2015/04/23 03:33:47, gzobqq wrote: > > On 2015/04/22 18:52:51, Nico (OOO sick) wrote: > > > On 2015/04/22 18:51:18, gzobqq wrote: > > > > On 2015/04/22 18:48:30, gzobqq wrote: > > > > > inferno: Thanks for explaining. > > > > > > > > > > Now, the trybot wants my entry in AUTHORS. I'd rather stay anonymous. I > > > could > > > > > add for example "Gzob Qq <mailto:gzobqq@gmail.com>", but it's not my > real > > > > name. Would > > > > > that be ok? > > > > > > > > Ah nevermind, CQ worked anyway. > > > > > > Oh, that's a great point actually. Did you sign the CLA? See > > > > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > > I think that requires a real name, and without this we can't take your > patch. > > > (Sorry, should've checked that in addition to the code) > > > > Yeah, sorry for not bringing it up before. I did sign the CLA with my real > name. > > I'd rather not add it to the public AUTHORS though. > > If you signed the CLA, I think it's ok to add some pseudonym to the authors file > as long as you also add the same email address that you put in the CLA – would > that work for you? Yeah, that is ok. I will create a new CL for that.
Message was sent while issue was closed.
On 2015/04/23 03:50:40, gzobqq wrote: > On 2015/04/23 03:40:58, Nico (OOO sick) wrote: > > On 2015/04/23 03:33:47, gzobqq wrote: > > > On 2015/04/22 18:52:51, Nico (OOO sick) wrote: > > > > On 2015/04/22 18:51:18, gzobqq wrote: > > > > > On 2015/04/22 18:48:30, gzobqq wrote: > > > > > > inferno: Thanks for explaining. > > > > > > > > > > > > Now, the trybot wants my entry in AUTHORS. I'd rather stay anonymous. > I > > > > could > > > > > > add for example "Gzob Qq <mailto:gzobqq@gmail.com>", but it's not my > > real > > > > > name. Would > > > > > > that be ok? > > > > > > > > > > Ah nevermind, CQ worked anyway. > > > > > > > > Oh, that's a great point actually. Did you sign the CLA? See > > > > > > > > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > > > I think that requires a real name, and without this we can't take your > > patch. > > > > (Sorry, should've checked that in addition to the code) > > > > > > Yeah, sorry for not bringing it up before. I did sign the CLA with my real > > name. > > > I'd rather not add it to the public AUTHORS though. > > > > If you signed the CLA, I think it's ok to add some pseudonym to the authors > file > > as long as you also add the same email address that you put in the CLA > – would > > that work for you? > > Yeah, that is ok. I will create a new CL for that. Did that ever happen?
Message was sent while issue was closed.
On 2015/04/29 03:57:51, Nico wrote: > On 2015/04/23 03:50:40, gzobqq wrote: > > On 2015/04/23 03:40:58, Nico (OOO sick) wrote: > > > On 2015/04/23 03:33:47, gzobqq wrote: > > > > On 2015/04/22 18:52:51, Nico (OOO sick) wrote: > > > > > On 2015/04/22 18:51:18, gzobqq wrote: > > > > > > On 2015/04/22 18:48:30, gzobqq wrote: > > > > > > > inferno: Thanks for explaining. > > > > > > > > > > > > > > Now, the trybot wants my entry in AUTHORS. I'd rather stay > anonymous. > > I > > > > > could > > > > > > > add for example "Gzob Qq <mailto:gzobqq@gmail.com>", but it's not my > > > real > > > > > > name. Would > > > > > > > that be ok? > > > > > > > > > > > > Ah nevermind, CQ worked anyway. > > > > > > > > > > Oh, that's a great point actually. Did you sign the CLA? See > > > > > > > > > > > > > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > > > > I think that requires a real name, and without this we can't take your > > > patch. > > > > > (Sorry, should've checked that in addition to the code) > > > > > > > > Yeah, sorry for not bringing it up before. I did sign the CLA with my real > > > name. > > > > I'd rather not add it to the public AUTHORS though. > > > > > > If you signed the CLA, I think it's ok to add some pseudonym to the authors > > file > > > as long as you also add the same email address that you put in the CLA > > – would > > > that work for you? > > > > Yeah, that is ok. I will create a new CL for that. > > Did that ever happen? It did now. Thanks for reminding me.
Message was sent while issue was closed.
AUTHORS CL link: https://codereview.chromium.org/1117763002/
Message was sent while issue was closed.
On 2015/04/29 19:43:18, gzobqq wrote: > AUTHORS CL link: https://codereview.chromium.org/1117763002/ Thanks this is great <a href="http://snapchatloginonline.co">Snapchat Login</a>
Message was sent while issue was closed.
On 2017/06/24 20:24:38, parmar.priyanka22 wrote: > On 2015/04/29 19:43:18, gzobqq wrote: > > AUTHORS CL link: https://codereview.chromium.org/1117763002/ > > Thanks this is great http://snapchatloginonline.co
Message was sent while issue was closed.
On 2017/06/24 20:25:00, parmar.priyanka22 wrote: > On 2017/06/24 20:24:38, parmar.priyanka22 wrote: > > On 2015/04/29 19:43:18, gzobqq wrote: > > > AUTHORS CL link: https://codereview.chromium.org/1117763002/ > > > > Thanks this is great http://snapchatloginonline.co > > Thank u so much for this one my brother :D http://uktvnow-apk.com |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
