Add a simple Poison class lifetime checker

RESOLVED FIXED in Firefox 49

Status

()

Core
MFBT
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
We've got a lot of un-reproducible crashes in graphics where I suspect we've got corruption occurring. Either use-after-free or out-of-bound writes.

I'm proposing we add mozilla::Poison which we can catch corruption earlier. Right now in some cases we hit the corruption when we try to access the vtable pointer and the crash address are all over the code. With mozilla::Poison we can confirm these theories as well as get better crash signature.
(Assignee)

Comment 1

2 years ago
Created attachment 8746226 [details]
MozReview Request: Bug 1268246 - Add a simple Poison class lifetime checker. r=froydnj

Review commit: https://reviewboard.mozilla.org/r/49325/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49325/
Attachment #8746226 - Flags: review?(nfroyd)
(Assignee)

Updated

2 years ago
Blocks: 1251615
https://reviewboard.mozilla.org/r/49325/#review46165

::: mfbt/Poison.h:82
(Diff revision 1)
> +    mValue = mozPoisonValue();
> +  }
> +
> +  void Check() {
> +    if (mValue != 0x12341234) {
> +      MOZ_CRASH("Poison check failed, check lifetime");

Drive-by comment: maybe document that this only catches double-frees, and to catch use-after-frees you have to add Check() calls to places where you suspect use-after-free might be occurring?
Comment on attachment 8746226 [details]
MozReview Request: Bug 1268246 - Add a simple Poison class lifetime checker. r=froydnj

https://reviewboard.mozilla.org/r/49325/#review46313

I'm interested to see if this makes any difference.  r=me with the changes below.

::: mfbt/Poison.h:62
(Diff revision 1)
>  extern MFBT_DATA uintptr_t gMozillaPoisonBase;
>  extern MFBT_DATA uintptr_t gMozillaPoisonSize;
>  
>  MOZ_END_EXTERN_C
>  
> +namespace mozilla {

Since we have MOZ_BEGIN_EXTERN_C bits in this header, let's wrap this namespace bit in #if defined(__cplusplus).

::: mfbt/Poison.h:65
(Diff revision 1)
> + * Insert as a class member to check that the class is in a valid state.
> + * This will help catch some use-after-free and out-f-bound writes.
> + * You can use Check() to verify the value of the class.

This comment requires a little more elaboration.  Taking Botond's suggestion into account, perhaps:

This class is designed to cause crashes when various kinds of memory corruption are observed. For instance, let's say we have a class C where we suspect out-of-bounds writes to some members.  We can insert a member of type Poison near the members we suspect are being corrupted by out-of-bounds writes.  Or perhaps we have a class K we suspect is subject to use-after-free violations, in which case it doesn't particularly matter where in the class we add the member of type Poison.

In either case, we then insert calls to Check() throughout the code.  Doing so enables us to narrow down the location where the corruption is occurring.  A pleasant side-effect of these additional Check() calls is that crash signatures may become more regular, as crashes will ideally occur consolidated at the point of a Check(), rather than scattered about at various uses of the corrupted memory.

::: mfbt/Poison.h:69
(Diff revision 1)
> +/**
> + * Insert as a class member to check that the class is in a valid state.
> + * This will help catch some use-after-free and out-f-bound writes.
> + * You can use Check() to verify the value of the class.
> + */
> +class Poison {

This is veering towards bikeshedding territory, but I think a better name for this class is CorruptionCanary.

::: mfbt/Poison.h:72
(Diff revision 1)
> + * You can use Check() to verify the value of the class.
> + */
> +class Poison {
> +public:
> +  Poison() {
> +    mValue = 0x12341234;

The chosen value should be a private static const member, so we don't have to duplicate it here and in the destructor.  Why don't we make this 0x0f0b0f0b while we're at it.

::: mfbt/Poison.h:80
(Diff revision 1)
> +  ~Poison() {
> +    Check();
> +    mValue = mozPoisonValue();
> +  }
> +
> +  void Check() {

Please make this const in this patch, rather than your patch which uses it.
Attachment #8746226 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 4

2 years ago
> This is veering towards bikeshedding territory, but I think a better name for this class is CorruptionCanary.

I agree. I realized the name wasn't good after discussing this with Botond.

> Please make this const in this patch, rather than your patch which uses it.

Yes, that was my plan. I did a self-review comment that I would fix this here but it didn't publish.

> The chosen value should be a private static const member, so we don't have to duplicate it here and in the destructor.  Why don't we make this 0x0f0b0f0b while we're at it.

I'm considering that it might be even better to use a random uuid style value here. But maybe that's overkill at this point. I'm fine with any value however a value like 0x12341234 stands out more in a crash report if we were ever going to incorrect deference this value.
(In reply to Benoit Girard (:BenWa) from comment #4)
> > This is veering towards bikeshedding territory, but I think a better name for this class is CorruptionCanary.
> 
> I agree. I realized the name wasn't good after discussing this with Botond.

Excellent!

> > The chosen value should be a private static const member, so we don't have to duplicate it here and in the destructor.  Why don't we make this 0x0f0b0f0b while we're at it.
> 
> I'm considering that it might be even better to use a random uuid style
> value here. But maybe that's overkill at this point. I'm fine with any value
> however a value like 0x12341234 stands out more in a crash report if we were
> ever going to incorrect deference this value.

My thought here was to try and avoid something that attackers could use as a nop sled.  0x0f0b0f0b is UD2 (repeated twice) on x86ish processors.  I don't know whether it actually matters that much in practice, though.
(Assignee)

Comment 6

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #5)
> 
> My thought here was to try and avoid something that attackers could use as a
> nop sled.  0x0f0b0f0b is UD2 (repeated twice) on x86ish processors.  I don't
> know whether it actually matters that much in practice, though.

I did consider that but if we really have something going wrong I don't think trying to harden a canary against is that valuable. Especially the 'safe' canary value. But I changed it to that value anyways since there's no harm in having it other than it providing a false sense of security.
(Assignee)

Comment 7

2 years ago
We might be able to extend the canary to do simple dynamic check for use-after-move. But I'm not familiar enough with the semantics so I'll leave it for now. If someone wants to take a look feel free to!
(Assignee)

Comment 8

2 years ago
Comment on attachment 8746226 [details]
MozReview Request: Bug 1268246 - Add a simple Poison class lifetime checker. r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49325/diff/1-2/
(Assignee)

Updated

2 years ago
Assignee: nobody → bgirard

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/506eacf36f93

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/506eacf36f93
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

a year ago
See Also: → bug 1305375
You need to log in before you can comment on or make changes to this bug.