1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE

Status: RESOLVED FIXED

(Core :: MFBT, defect, P3)

Description

[Lars T Hansen [:lth]]

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.

Comment 1

[HellosAazS]

Hello!May I ask if someone is working on this bug?
I'm a newbie and would like to give this a try!

Comment 2

[Steve Fink [:sfink] [:s:]]

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.

Comment 3

[HellosAazS]

Attachment: Bug1353767.patch

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

[Steve Fink [:sfink] [:s:]]

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

[Steve Fink [:sfink] [:s:]]

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.

Comment 6

[HellosAazS]

(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!

Comment 7

[HellosAazS]

Attachment: BufferList methods that can OOM should all be MOZ_MUST_USE

Hello,I've create the patch,hope this work!

Comment 8

[Chris Peterson [:cpeterson]]

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.

Comment 9

[Steve Fink [:sfink] [:s:]]

I pushed the patch to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd97904f78dda3452a988846f3cd99ff3a899834

Comment 10

[HellosAazS]

(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!

Comment 11

[HellosAazS]

Attachment: Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE

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!

Comment 12

[Chris Peterson [: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.

Comment 13

[Steve Fink [:sfink] [:s:]]

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.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 15

[Steve Fink [:sfink] [:s:]]

(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.)

Comment 16

[HellosAazS]

Attachment: Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE

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!

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 18

[HellosAazS]

(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.

Comment 19

[HellosAazS]

Attachment: Bug1353767.patch

Good day,I've use MOZ_ALWAYS_TRUE on those WriteBytes!

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

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?

Comment 21

[Andrew McCreight [:mccr8]]

I can rebase and land the patch.

Comment 22

[Andrew McCreight [:mccr8]]

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.

Comment 23

[Andrew McCreight [:mccr8]]

Attachment: BufferList methods that can OOM should all be MOZ_MUST_USE.

The Pickle methods can use MOZ_ALWAYS_TRUE because the BufferList is
infallible, so the WriteBytes calls will never fail.

Comment 24

[Andrew McCreight [:mccr8]]

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.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba34ca57b5b
BufferList methods that can OOM should all be MOZ_MUST_USE. r=billm

Comment 26

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3ba34ca57b5b

Comment 27

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/projects/ash/rev/3ba34ca57b5b5c73a07a026a6103cbc7394c2207
Bug 1353767 - BufferList methods that can OOM should all be MOZ_MUST_USE. r=billm