Closed
Bug 1353767
Opened 7 years ago
Closed 5 years ago
BufferList methods that can OOM should all be MOZ_MUST_USE
Categories
(Core :: MFBT, defect, P3)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla68
People
(Reporter: lth, Assigned: HellosAazS)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
I ran into a case in js/src/vm/StructuredClone.cpp where the return value from WriteBytes was not checked. To safeguard against that type of error, the methods that can OOM should all be marked MOZ_MUST_USE and the callers should be updated.
Assignee | ||
Comment 1•7 years ago
|
||
Hello!May I ask if someone is working on this bug? I'm a newbie and would like to give this a try!
Flags: needinfo?(lhansen)
Comment 2•7 years ago
|
||
Nobody is working on it. Please, go ahead! You'll be looking at mfbt/BufferList.h and tracing through everything that can call mSegments.append() (directly or indirectly), and marking those with MOZ_MUST_USE eg template<typename AllocPolicy> MOZ_MUST_USE bool BufferList<AllocPolicy>::WriteBytes(const char* aData, size_t aSize) Then try to compile, and fix the warnings that result.
Blocks: use-nodiscard
Flags: needinfo?(lhansen)
Assignee | ||
Comment 3•7 years ago
|
||
Hello! I've add the MOZ_MUST_USE to those methods in mfbt/BufferList.h. But I couldn't get any warnings when I use "./mach build binaries" command to rebuild the code.Did I miss something?Thanks for your reply!
Comment 4•7 years ago
|
||
Oh, sorry! I just learned that MOZ_MUST_USE only works with gcc and clang. Ugh. How about... I'll give you comments on the patch, then you can use http://searchfox.org/mozilla-central/search?q=BufferList%3A%3AWriteBytes&path= to find everything that doesn't check the return value and fix it, then I'll push your updated patch to try server. If it fails, I'll give you a link to the compile log. Thanks, and sorry for the trouble.
Comment 5•7 years ago
|
||
Comment on attachment 8861250 [details] [diff] [review] Bug1353767.patch Review of attachment 8861250 [details] [diff] [review]: ----------------------------------------------------------------- So in the end, it looks like WriteBytes was the only one requiring the annotation. I didn't look to see how many callers need to be updated. ::: mfbt/BufferList.h @@ +279,1 @@ > BorrowingAllocPolicy aAP = BorrowingAllocPolicy()) const; I would only add MOZ_MUST_USE to things that return a bool or a possibly NULL pointer. (There's no point in calling eg Borrow() without using the return value. So it doesn't hurt, but it does clutter up the code a little.) What we really want here is to ensure that *aSuccess is checked, but MOZ_MUST_USE is insufficient for that. I filed bug 1359562 -- we might already have a bug for it, but I couldn't find one. @@ +312,5 @@ > mStandardCapacity(0) > { > } > > + MOZ_MUST_USE void* AllocateSegment(size_t aSize, size_t aCapacity) I probably wouldn't annotate this one either. It's true that the caller really ought to use the result, but it's very unlikely that a caller *wouldn't* use it.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #4) > Oh, sorry! I just learned that MOZ_MUST_USE only works with gcc and clang. > > Ugh. How about... I'll give you comments on the patch, then you can use > http://searchfox.org/mozilla-central/ > search?q=BufferList%3A%3AWriteBytes&path= to find everything that doesn't > check the return value and fix it, then I'll push your updated patch to try > server. If it fails, I'll give you a link to the compile log. > > Thanks, and sorry for the trouble. Will do that!Thank you for your reply and comments!
Assignee | ||
Comment 7•7 years ago
|
||
Hello,I've create the patch,hope this work!
Attachment #8861250 -
Attachment is obsolete: true
Flags: needinfo?(sphink)
Comment 8•7 years ago
|
||
Thanks! Your patch looks good. Two minor comments: 1. I think you might have some tab characters in your patch. Firefox's coding style is to indent using spaces instead of tabs. Each C++ indentation level should be two spaces (except in the js/ directory where four spaces are used for historical reasons). 2. I think there are some more calls to WriteBytes() in mfbt/tests/TestBufferList.cpp. Since that is a test file, you can just check the WriteBytes() return value using MOZ_RELEASE_ASSERT(WriteBytes()). MOZ_RELEASE_ASSERT() is not usually a good way to handle errors because it will crash the application, but that shouldn't be a problem here because WriteBytes() should not run out of memory while running this short test.
Assignee: nobody → HellosAazS
Priority: -- → P3
Comment 9•7 years ago
|
||
I pushed the patch to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd97904f78dda3452a988846f3cd99ff3a899834
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #9) > I pushed the patch to try: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=bd97904f78dda3452a988846f3cd99ff3a899834 Thanks for the help,I'll fix them!
Assignee | ||
Comment 11•7 years ago
|
||
Thanks for the comments, I've fixed them! Could you help review the patch or tell me who should I set the review flag to? Thank you!
Attachment #8863176 -
Attachment is obsolete: true
Flags: needinfo?(sphink) → needinfo?(cpeterson)
Comment 12•7 years ago
|
||
Comment on attachment 8864516 [details] [diff] [review] Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE Review of attachment 8864516 [details] [diff] [review]: ----------------------------------------------------------------- sfink, can you please take a look at HellosAazS' new patch when you have a chance? ::: ipc/chromium/src/base/pickle.cc @@ +501,5 @@ > kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, > kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, > }; > + if (!buffers_.WriteBytes(padding_data, padding)) { > + return; Perhaps all these Pickle member functions should be returning a success/failure value? It seems like a bad idea that a Pickle object could hold incomplete data without the caller knowing.
Attachment #8864516 -
Flags: review?(sphink)
Updated•7 years ago
|
Flags: needinfo?(cpeterson)
Comment 13•7 years ago
|
||
Comment on attachment 8864516 [details] [diff] [review] Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE Review of attachment 8864516 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, sorry, I glanced at the patch and the changes in pickle.cc made me want to go back and figure out how error handling should be done there. But I'm unfamiliar with what that code is used for and don't have time to dig in right now, so I'll redirect the review to the person who has reviewed it the most in the past.
Attachment #8864516 -
Flags: review?(sphink) → review?(wmccloskey)
Comment on attachment 8864516 [details] [diff] [review] Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE Review of attachment 8864516 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/chromium/src/base/pickle.cc @@ +500,5 @@ > static const char padding_data[8] = { > kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, > kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, > }; > + if (!buffers_.WriteBytes(padding_data, padding)) { Please just MOZ_ASSERT or something here. BufferList uses InfallibleAllocPolicy, so there's no way that this could fail. Also, please don't MOZ_RELEASE_ASSERT. It's not worth the cost.
Attachment #8864516 -
Flags: review?(wmccloskey) → review+
Comment 15•7 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #14) > Comment on attachment 8864516 [details] [diff] [review] > Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE > > Review of attachment 8864516 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: ipc/chromium/src/base/pickle.cc > @@ +500,5 @@ > > static const char padding_data[8] = { > > kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, > > kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, > > }; > > + if (!buffers_.WriteBytes(padding_data, padding)) { > > Please just MOZ_ASSERT or something here. BufferList uses > InfallibleAllocPolicy, so there's no way that this could fail. Ah, ok. But to be specific, *this* BufferList uses InfallibleAllocPolicy; the MOZ_MUST_USE annotation is still useful because other instantations don't. (If we were clever, we would use some complicated scheme to make MOZ_MUST_USE only apply to other specializations. But that would be messy.) > Also, please don't MOZ_RELEASE_ASSERT. It's not worth the cost. Use MOZ_ALWAYS_TRUE. (That won't do anything in a non-DEBUG build.)
Assignee | ||
Comment 16•7 years ago
|
||
Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE Hello,I've add the MOZ_ASSERT and use the MOZ_ALWAYS_TRUE instead of MOZ_RELEASE_ASSERT, could you please help take a look at it? Thank you!
Attachment #8864516 -
Attachment is obsolete: true
Attachment #8868069 -
Flags: review?(wmccloskey)
Comment on attachment 8868069 [details] [diff] [review] Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE Review of attachment 8868069 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I confused you saying you should use MOZ_ASSERT. Steve is right, this should use MOZ_ALWAYS_TRUE. ::: ipc/chromium/src/base/pickle.cc @@ +500,5 @@ > static const char padding_data[8] = { > kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, > kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, > }; > + MOZ_ASSERT(buffers_.WriteBytes(padding_data, padding)); You need to use MOZ_ALWAYS_TRUE here. Otherwise WriteBytes won't be called in non-DEBUG builds. @@ +517,5 @@ > MOZ_RELEASE_ASSERT(padding <= 4); > static const char padding_data[4] = { > kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, kBytePaddingMarker, > }; > + MOZ_ASSERT(buffers_.WriteBytes(padding_data, padding)); Same as above. @@ +527,5 @@ > DCHECK(intptr_t(header_) % alignment == 0); > > BeginWrite(data_len, alignment); > > + MOZ_ASSERT(buffers_.WriteBytes(reinterpret_cast<const char*>(data), data_len)); Same. @@ +578,5 @@ > #endif > } > > void Pickle::InputBytes(const char* data, uint32_t length) { > + MOZ_ASSERT(buffers_.WriteBytes(data, length)); Same.
Attachment #8868069 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #17) > Comment on attachment 8868069 [details] [diff] [review] > Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE > > Review of attachment 8868069 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry I confused you saying you should use MOZ_ASSERT. Steve is right, this > should use MOZ_ALWAYS_TRUE. > Sorry! I misunderstand what you say, I'll fix them.Thank you.
Assignee | ||
Comment 19•7 years ago
|
||
Good day,I've use MOZ_ALWAYS_TRUE on those WriteBytes!
Attachment #8868069 -
Attachment is obsolete: true
Attachment #8869709 -
Flags: review?(wmccloskey)
Attachment #8869709 -
Flags: review?(wmccloskey) → review+
Comment 20•5 years ago
|
||
There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:HellosAazS, could you have a look please?
Flags: needinfo?(HellosAazS)
Comment 21•5 years ago
|
||
I can rebase and land the patch.
Flags: needinfo?(HellosAazS) → needinfo?(continuation)
Comment 22•5 years ago
|
||
This patch is basically the same as before, except that the changes in StructuredCloneData are no longer present. I have a followup patch that will do the equivalent in the new code, I think.
Flags: needinfo?(continuation)
Comment 23•5 years ago
|
||
The Pickle methods can use MOZ_ALWAYS_TRUE because the BufferList is infallible, so the WriteBytes calls will never fail.
Attachment #9053727 -
Flags: review+
Comment 24•5 years ago
|
||
Comment on attachment 8869709 [details] [diff] [review] Bug1353767.patch I just rebased the patch and carried forward billm's old review. I can't use Phabricator without getting somebody else's review, so I've just uploaded the patch and I'll mark checkin needed.
Attachment #8869709 -
Attachment is obsolete: true
Updated•5 years ago
|
Keywords: checkin-needed
Comment 25•5 years ago
|
||
Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba34ca57b5b
BufferList methods that can OOM should all be MOZ_MUST_USE. r=billm
Keywords: checkin-needed
Comment 26•5 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox68:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Comment 27•5 years ago
|
||
https://hg.mozilla.org/projects/ash/rev/3ba34ca57b5b5c73a07a026a6103cbc7394c2207 Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE. r=billm
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•