Closed Bug 1353767 Opened 3 years ago Closed 8 months ago

BufferList methods that can OOM should all be MOZ_MUST_USE

Categories

(Core :: MFBT, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox55 --- wontfix
firefox68 --- fixed

People

(Reporter: lth, Assigned: HellosAazS)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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.
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)
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: MOZ_MUST_USE
Flags: needinfo?(lhansen)
Attached patch Bug1353767.patch (obsolete) — Splinter Review
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!
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 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.
(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!
Hello,I've create the patch,hope this work!
Attachment #8861250 - Attachment is obsolete: true
Flags: needinfo?(sphink)
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
(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!
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 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)
Flags: needinfo?(cpeterson)
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+
(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.)
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)
(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.
Attached patch Bug1353767.patch (obsolete) — Splinter Review
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+

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)

I can rebase and land the patch.

Flags: needinfo?(HellosAazS) → needinfo?(continuation)

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)
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 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
Blocks: 1539261

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
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
https://hg.mozilla.org/projects/ash/rev/3ba34ca57b5b5c73a07a026a6103cbc7394c2207
Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE. r=billm
You need to log in before you can comment on or make changes to this bug.