Closed Bug 968823 Opened 6 years ago Closed 6 years ago

Shmem::Alloc and Shmem::OpenExisting should not abort on bad params

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

Attachments

(1 file)

Found this while fuzzing IPC with Faulty. See dependent bugs. I only hit crashes in OpenExisting(), but looking at this file, Alloc() also is to be called by IPDL-generated code and to be passed untrusted param values. These functions should not abort (note that they abort even in release builds) based on untrusted parameter values.
Attachment #8372439 - Flags: review?(bent.mozilla)
Comment on attachment 8372439 [details] [diff] [review]
Make Shmem more graceful

Review of attachment 8372439 [details] [diff] [review]:
-----------------------------------------------------------------

This is opening up some previously untested code paths, are you sure callers can actually handle these null values appropriately?

::: ipc/glue/Shmem.cpp
@@ +370,5 @@
>    else if (aType == SharedMemory::TYPE_SYSV)
>      segment = CreateSegment(segmentSize, SharedMemorySysV::NULLHandle());
>  #endif
> +  else {
> +    NS_WARNING("unknown shmem type");

Hrm, if we're making this warn instead of crash (or even assert) could you please file a followup to make this do whatever we decide to do for fuzzing assertions? I'm worried that we'll forget about these.

@@ +513,1 @@
>      // Unhandled!!

Nit: This comment isn't valid any more.

@@ +513,2 @@
>      // Unhandled!!
> +    NS_WARNING("unhandled shmem alloc!");

Odd, but this is inside a #ifndef DEBUG block, so this warning and all the rest of these warnings below are never going to be used. Just nuke them.

@@ +597,4 @@
>  #endif  // if defined(DEBUG)
>  
>  int
>  Shmem::GetSysVID() const

FYI it looks like this is entirely unused. Feel free to remove if try is happy?

@@ +636,5 @@
>      return new ShmemCreated(routingId, mId, mSize, seg->GetHandle());
>    }
>  #endif
>    else {
> +    NS_ABORT_IF_FALSE(false, "unknown shmem type (here?!)");

Did you mean to leave this as an abort after changing the rest to warnings?
Comment on attachment 8372439 [details] [diff] [review]
Make Shmem more graceful

Review of attachment 8372439 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with previous comments addressed.
Attachment #8372439 - Flags: review?(bent.mozilla) → review+
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #2)
> Comment on attachment 8372439 [details] [diff] [review]
> Make Shmem more graceful
> 
> Review of attachment 8372439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is opening up some previously untested code paths, are you sure callers
> can actually handle these null values appropriately?
> 
> ::: ipc/glue/Shmem.cpp
> @@ +370,5 @@
> >    else if (aType == SharedMemory::TYPE_SYSV)
> >      segment = CreateSegment(segmentSize, SharedMemorySysV::NULLHandle());
> >  #endif
> > +  else {
> > +    NS_WARNING("unknown shmem type");
> 
> Hrm, if we're making this warn instead of crash (or even assert) could you
> please file a followup to make this do whatever we decide to do for fuzzing
> assertions? I'm worried that we'll forget about these.

In fact, good catch: I've replaced NS_WARNING by NS_ERROR in my patch so that if this actually happens while running tests, this will recorded like an assertion failure. Is that enough? (By default I suppose that it is).

> >  
> >  int
> >  Shmem::GetSysVID() const
> 
> FYI it looks like this is entirely unused. Feel free to remove if try is
> happy?

This is used in a unit test: ipc/ipdl/test/cxx/TestSysVShmem.cpp

> 
> @@ +636,5 @@
> >      return new ShmemCreated(routingId, mId, mSize, seg->GetHandle());
> >    }
> >  #endif
> >    else {
> > +    NS_ABORT_IF_FALSE(false, "unknown shmem type (here?!)");
> 
> Did you mean to leave this as an abort after changing the rest to warnings?

Actually, yes. That is because, if I understand correctly, the "(here?!)" in this message is saying that this place is internal enough that any such "unknown shmem type" should have been validated against earlier, and it should be impossible for such bad state to exist at this point.
Assignee: nobody → bjacob
https://hg.mozilla.org/mozilla-central/rev/86350b7fe15f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.