Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
290173008:
Refactor PickleIterator. (Closed)
|
Created:
6 years, 7 months ago by halyavin Modified:
6 years, 6 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionRefactor PickleIterator.
Remove undefined behaviors by using offsets instead of pointers
and by checking that offset never exceeds the size of the data.
Update Native Messaging test to use Native Messaging format
instead of Pickle format which accidentally coincided before.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274367
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 18
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/290173008/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc (right): https://codereview.chromium.org/290173008/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc:146: return std::string(reinterpret_cast<char*>(&length), 4).append(message); The original code is confusing for other reasons... like why would you use const cast to add const-ness? ...but why can't that code still work?.... and why are you providing a separate implementation of the writing of a string (message) here? Why isn't the other code still functional?
https://codereview.chromium.org/290173008/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc (right): https://codereview.chromium.org/290173008/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc:146: return std::string(reinterpret_cast<char*>(&length), 4).append(message); On 2014/05/22 02:51:00, jar wrote: > The original code is confusing for other reasons... like why would you use const > cast to add const-ness? > > ...but why can't that code still work?.... and why are you providing a separate > implementation of the writing of a string (message) here? Why isn't the other > code still functional? I made Pickle payload size aligned to 4 bytes and hence the resulting string had extra zeros at the end.
ping
https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc File base/pickle.cc (right): https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode39 base/pickle.cc:39: inline void PickleIterator::Advance(size_t size) { I *think* all (both?) calls to this method pass in an aligned size (doing the alignment at the call site). It might be clearer to define this method to do the alignment for the caller, and hence it becomes very clear that the read_index remains aligned, until it matches end_index_. https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode50 base/pickle.cc:50: return NULL; I agree that you perfectly translate the existing code here. It is interesting that the old code didn't do the "Advance" part of the method name. Do you think it is correct to leave read_index_ intact, or should we do: read_index_ = end_index_; // Advancing anyway. The advantage(?) of that change is that it makes it harder (impossible?) to make further use of the iterator. https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode52 base/pickle.cc:52: Advance(AlignInt(sizeof(Type), sizeof(uint32_t))); I think the precise translation of the old code would be: if (sizeof(Type) < sizeof(int32)) Advance(AlignInt(sizeof(Type), sizeof(uint32_t))); else Advance(sizeof(Type)); I think you skipped the test because you are intentionally always doing the AlignInt now.... https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode59 base/pickle.cc:59: return NULL; Here again I *wonder* if we should advance, making the iterator unusable? https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode66 base/pickle.cc:66: size_t size_element) { nit: While you're here.... all arguments should be aligned... so you should put the argument from line 65 onto a new line, indented by 4, and then put this arg directly under it. https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode342 base/pickle.cc:342: header_->payload_size = static_cast<uint32>(new_size); Would this change alone, which keeps payload_size aligned, have resolved the issue with the iterator (getting an unaligned payload_size, and then going "well beyond" the payload_size when we did align, thereby creating an invalid pointer)? https://codereview.chromium.org/290173008/diff/60001/chrome/browser/extension... File chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc (right): https://codereview.chromium.org/290173008/diff/60001/chrome/browser/extension... chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc:146: return std::string(reinterpret_cast<char*>(&length), 4).append(message); Yes... SGTM. thx.
https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc File base/pickle.cc (right): https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode39 base/pickle.cc:39: inline void PickleIterator::Advance(size_t size) { On 2014/05/23 21:22:04, jar wrote: > I *think* all (both?) calls to this method pass in an aligned size (doing the > alignment at the call site). It might be clearer to define this method to do > the alignment for the caller, and hence it becomes very clear that the > read_index remains aligned, until it matches end_index_. Done. https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode50 base/pickle.cc:50: return NULL; On 2014/05/23 21:22:04, jar wrote: > I agree that you perfectly translate the existing code here. It is interesting > that the old code didn't do the "Advance" part of the method name. > > Do you think it is correct to leave read_index_ intact, or should we do: > read_index_ = end_index_; // Advancing anyway. > > The advantage(?) of that change is that it makes it harder (impossible?) to make > further use of the iterator. > Looks reasonable. Let us see what happens. https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode52 base/pickle.cc:52: Advance(AlignInt(sizeof(Type), sizeof(uint32_t))); On 2014/05/23 21:22:04, jar wrote: > I think the precise translation of the old code would be: > if (sizeof(Type) < sizeof(int32)) > Advance(AlignInt(sizeof(Type), sizeof(uint32_t))); > else > Advance(sizeof(Type)); > > I think you skipped the test because you are intentionally always doing the > AlignInt now.... It was a bug since sizeof(Type) may not be divisible by 4. sizeof(struct { short a; short b; short c;}) == 6 on Windows. https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode59 base/pickle.cc:59: return NULL; On 2014/05/23 21:22:04, jar wrote: > Here again I *wonder* if we should advance, making the iterator unusable? Done. https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode66 base/pickle.cc:66: size_t size_element) { On 2014/05/23 21:22:04, jar wrote: > nit: While you're here.... all arguments should be aligned... so you should put > the argument from line 65 onto a new line, indented by 4, and then put this arg > directly under it. Done. https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode342 base/pickle.cc:342: header_->payload_size = static_cast<uint32>(new_size); On 2014/05/23 21:22:04, jar wrote: > Would this change alone, which keeps payload_size aligned, have resolved the > issue with the iterator (getting an unaligned payload_size, and then going "well > beyond" the payload_size when we did align, thereby creating an invalid > pointer)? No, because the other side of IPC connection might be controlled by an attacker.
https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc File base/pickle.cc (right): https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode342 base/pickle.cc:342: header_->payload_size = static_cast<uint32>(new_size); On 2014/05/26 07:43:02, halyavin wrote: > On 2014/05/23 21:22:04, jar wrote: > > Would this change alone, which keeps payload_size aligned, have resolved the > > issue with the iterator (getting an unaligned payload_size, and then going > "well > > beyond" the payload_size when we did align, thereby creating an invalid > > pointer)? > No, because the other side of IPC connection might be controlled by an attacker. > Isn't this the side that is writing into a pickle, for transmission. Why does it matter if the recipient (other end of the IPC) is an attacker?? It seems that if we intentionally keep payload_size aligned, a recipient (of the pickle) can validate that fact (during iterator construction)... and never lose this property (during iterations). I think a pile of this CL related to having a payload size that was not aligned... and then having to do backflips to deal with it clearly to avoid having a pointer beyond an allocated region.
https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc File base/pickle.cc (right): https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode342 base/pickle.cc:342: header_->payload_size = static_cast<uint32>(new_size); > Isn't this the side that is writing into a pickle, for transmission. Why does > it matter if the recipient (other end of the IPC) is an attacker?? It doesn't. I meant that we still need to check alignment on the receiving side, if transmitting side is an attacker. > > I think a pile of this CL related to having a payload size that was not > aligned... and then having to do backflips to deal with it clearly to avoid > having a pointer beyond an allocated region. It is all concentrated in Advance method. We can easily remove it in the future. One more thing I am worrying about is Chrome components which update independently of Chrome (like PNaCl translator). If they contain an IPC or Pickle code, they will break on a non-backward-compatible change.
+cc Carlos, so we have extra eyes on this most critical code. LGTM... with nit below. ...but get Carlos's confirmation... then then Brett can probably review or stamp. https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc File base/pickle.cc (right): https://codereview.chromium.org/290173008/diff/60001/base/pickle.cc#newcode342 base/pickle.cc:342: header_->payload_size = static_cast<uint32>(new_size); On 2014/05/28 07:45:12, halyavin wrote: > > Isn't this the side that is writing into a pickle, for transmission. Why does > > it matter if the recipient (other end of the IPC) is an attacker?? > > It doesn't. I meant that we still need to check alignment on the receiving side, > if transmitting side is an attacker. > > > > > I think a pile of this CL related to having a payload size that was not > > aligned... and then having to do backflips to deal with it clearly to avoid > > having a pointer beyond an allocated region. > > It is all concentrated in Advance method. We can easily remove it in the future. > > One more thing I am worrying about is Chrome components which update > independently of Chrome (like PNaCl translator). If they contain an IPC or > Pickle code, they will break on a non-backward-compatible change. FWIW: I checked... PNaCL uses its own IPC, and handles such issues separately (no pickle involved). We always ship both side of the IPC in any upgrade... and I thought we were completely safe (with a binary change of pickle format),... but Darin pointed out that we persist history in a pickle format, and we'd need to edit special code *IF* we made a binary change. It is probably better not to bother with such. https://codereview.chromium.org/290173008/diff/80001/base/pickle.h File base/pickle.h (right): https://codereview.chromium.org/290173008/diff/80001/base/pickle.h#newcode85 base/pickle.h:85: const char* payload_ptr_; nit: Now that we're not advancing this ptr, perhaps just |payload_| would be a better name. Comments are often helpful. const char* payload_; // Start of our pickle's payload. size_t read_index_; // Next readable byte is payload_[read_index_]. size_t end_index_; // Index that is one char beyond the end of our payload_[].
https://codereview.chromium.org/290173008/diff/80001/base/pickle.h File base/pickle.h (right): https://codereview.chromium.org/290173008/diff/80001/base/pickle.h#newcode85 base/pickle.h:85: const char* payload_ptr_; On 2014/05/29 01:51:16, jar wrote: > nit: Now that we're not advancing this ptr, perhaps just |payload_| would be a > better name. > > Comments are often helpful. > > const char* payload_; // Start of our pickle's payload. > size_t read_index_; // Next readable byte is payload_[read_index_]. > size_t end_index_; // Index that is one char beyond the end of our payload_[]. Done.
Brett, I need your LGTM as an owner of base/. Sergey Ulanov, I need your LGTM as an owner of chrome/browser/extensions/api/messaging/.
Adding justin for security review. https://codereview.chromium.org/290173008/diff/100001/base/pickle.cc File base/pickle.cc (left): https://codereview.chromium.org/290173008/diff/100001/base/pickle.cc#oldcode45 base/pickle.cc:45: else the two versions do not seem equivalent. Maybe I am misreading it but the sizeof(Type) is no longer compared to sizeof(uint32). Looks like we are going to get different alignments, maybe not a problem per se.
https://codereview.chromium.org/290173008/diff/100001/base/pickle.cc File base/pickle.cc (left): https://codereview.chromium.org/290173008/diff/100001/base/pickle.cc#oldcode45 base/pickle.cc:45: else On 2014/05/29 17:14:58, cpu wrote: > the two versions do not seem equivalent. Maybe I am misreading it but the > sizeof(Type) is no longer compared to sizeof(uint32). > > Looks like we are going to get different alignments, maybe not a problem per se. It is intentional. The previous code was wrong.
owbers lgtm rubberstamp based on jar's review and pending security review.
lgtm
Security lgtm with one nit/question. https://codereview.chromium.org/290173008/diff/100001/base/pickle.cc File base/pickle.cc (right): https://codereview.chromium.org/290173008/diff/100001/base/pickle.cc#newcode70 base/pickle.cc:70: inline const char* PickleIterator::GetReadPointerAndAdvance( Nit: Not security related, but does the explicit inline really make sense here?
https://codereview.chromium.org/290173008/diff/100001/base/pickle.cc File base/pickle.cc (right): https://codereview.chromium.org/290173008/diff/100001/base/pickle.cc#newcode70 base/pickle.cc:70: inline const char* PickleIterator::GetReadPointerAndAdvance( On 2014/05/30 13:50:31, Justin Schuh wrote: > Nit: Not security related, but does the explicit inline really make sense here? I removed inline from *.h file. I leave it here so that it can be inlined in internal methods.
lgtm
The CQ bit was checked by halyavin@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/halyavin@google.com/290173008/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 274367 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
