Add mfbt pointer wrapper to manage clang's no_sanitize("vptr")

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

(Blocks 1 bug)

61 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

a year ago
Spawned from bug 1442819 comment 14 by Nathan:
> ::: dom/media/doctor/DecoderDoctorLogger.h:483
> > +  // This constructor is called before the `T` object is fully constructed, and
> > +  // pointers are not dereferenced anyway, so UBSan shouldn't check vptrs.
> > +  __attribute__((no_sanitize("vptr")))
> 
> Can you file a followup to add a proper attribute in `mfbt/Attributes.h` for this sort of thing?
Ugh, what a mess (looking at that bug).

I don't think we want to give people this tool, disconnected from particular use cases.  It would be pretty easy for anyone not deeply versed in C++ to misuse it, if we provide it.

So here's a counterproposal: how about we add (to mfbt/Casting.h) a function *specifically designed* for casting a base pointer to an incompletely-initialized derived pointer?

/**
 * Given a pointer to a base-class object, return a pointer to a derived-class
 * object that inherits from it *when that derived-class object may not be fully
 * initialized*.  (Such pointers are still safely usable in limited ways, such
 * as for their numeric value and for those subobjects that *have* been
 * initialized.)
 *
 * This function exists to *explicitly* indicate that the returned derived-class
 * object *is not fully usable*.  This function also plays nice with
 * undefined-behavior sanitizers that will sometimes check whether the object
 * being cast actually *is* of the derived type (which checks can only safely be
 * performed if the derived object is fully usable).  Otherwise its semantics
 * are identical to those of |static_cast<Derived*>(base)|.
 *
 * The most common use of this function is to cast to a derived class within a
 * base class constructor or destructor:
 *
 *   * The derived object is only partially initialized inside a base class
 *     *constructor* because the derived constructor is only called *after* the
 *     base class constructor.  Derived class fields are not yet initialized,
 *     and other base classes may or may not yet be initialized.
 *   * The derived object is  only partially initialized inside a base class
 *     *destructor* because the derived destructor is called *before* the base
 *     class destructor, leaving all derived fields uninitialized and other base
 *     classes possibly uninitialized.
 */
template<class Derived, class Base>
#if defined(__clang__)
__attribute__((no_sanitize("vptr")))
#endif
static inline Derived*
CastToPartiallyInitializedDerived(Base* aBase)
{
  static_assert(IsBaseOf<Base, Derived>::value,
                "base-derived inheritance relationship must exist between Base "
                "and Derived");
  return static_cast<Derived*>(aBase);
}

I would much prefer this to a general-purpose attribute susceptible to misuse to silence serious warnings and errors.  A function like the above, on the other hand, gives some indication as to what it will produce and when it may be used.
Assignee

Comment 2

a year ago
Thank you Jeff for your suggestion.

If we're going for minimal potential harm, I'd say we could go even further:
Instead of returning an alluringly-dereferencable Derived* pointer, how about we just return void*? -- Selfishly, that's all I require. ;-)

If a real need to keep a typed pointer comes up later, we can think longer&harder about it then...

Before I implement this (or something else, depending on further discussion), I'll have a look at other UBSan downcast issues [1], in case they do uncover other surprises.


[1] https://bugzilla.mozilla.org/buglist.cgi?short_desc=UBSan%20downcast&bug_status=NEW&bug_status=ASSIGNED&bug_status=RESOLVED&short_desc_type=allwordssubstr
That wouldn't actually be sufficient for your use case -- you want to pass a well-typed pointer so that you have the type of the pointee (that isn't being passed redundantly), so you can pass that to a traits class, so you can get a printable name, and stuff like that.  You *could* pass the derived-class type manually, to be sure, but that's somewhat duplicative arguably.

https://searchfox.org/mozilla-central/source/dom/media/doctor/DecoderDoctorLogger.h#290
https://searchfox.org/mozilla-central/source/dom/media/doctor/DecoderDoctorLogger.h#321

But I agree -- it would be preferable if a function like this returned void* instead of the derived type, when pointer identity is all the pointer's really used for.  Just, here the actual type is used statically and without respect to the C++ object model, and so just having void* only isn't enough.
Assignee

Comment 4

a year ago
(In reply to Jeff Walden [:Waldo] from comment #3)
> That wouldn't actually be sufficient for your use case [...]  You *could* pass the
> derived-class type manually, to be sure, but that's somewhat duplicative
> arguably.

I would call `LogConstruction(const char* aSubjectTypeName, const void* aSubjectPointer)` directly. ;-)
Yes, it would add a bit of duplication, but I think it makes sense in this file (the type-traits are accessed from multiple places already, one more won't do much harm), and it will then be more demonstrably safe with regards to dangerous pointers.

> But I agree -- it would be preferable if a function like this returned void*
> instead of the derived type, when pointer identity is all the pointer's
> really used for.  Just, here the actual type is used statically and without
> respect to the C++ object model, and so just having void* only isn't enough.

I'm unclear about your very last sentence.
If "here" refers to DecoderDoctorLogger, I think I can work with it as explained above.
If "here" means something else, please clarify.
Assignee

Comment 5

a year ago
I naively thought I could just go with my version of CastToPartiallyInitializedDerived returning void*, but I encountered some complications in DecoderDoctorLogger when I needed to do a further cast to another related class!

So I ended up stealing the idea of annotating pointers from NotNull, with a class that contains a pointer that should not be dereferenced.
Extra benefits: It actively prevents dereferencing, but still allows casts (with no UBSan complaints).

Nathan, I'll push patches for review soon, feel free to reject if my proposal is not appropriate, or find another reviewer if you're busy.
Other comments always welcome. (Jeff?)


Recent try for the curious: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d518b19c42a1d9feebc3a144cd4d280dc96d0329
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
Comment on attachment 8964746 [details]
Bug 1448494 - NonDereferenceable<T> wraps a T* and prevents dereferencing ops -

https://reviewboard.mozilla.org/r/233470/#review240606

This wound up being a lot more machinery than I expected.  I'm not confident all of it is needed, but I suppose explicit types are better than random compiler attributes.

r- just to make sure we talk through all of the issues, especially the `explicit` and `value()` return type, but I don't see anything actually objectionable here.

::: commit-message-a1fb8:1
(Diff revision 1)
> +Bug 1448494 - NonDereferencable<T> wraps a T* and prevents dereferencing ops - r?froydnj

Nit: I think this is supposed to be `NonDereferenceable`, see e.g. [wiktionary](https://en.wiktionary.org/wiki/dereferencable).  Here and throughout the patch.

::: commit-message-a1fb8:6
(Diff revision 1)
> +Dereferencing operations are explicitly disabled to avoid unintentional
> +misuses.

Nit: this is sort of weird word wrapping.  Move "misuses" to the previous line?

::: commit-message-a1fb8:12
(Diff revision 1)
> +misuses.
> +Casting is still possible between related types (same as with raw pointers),
> +but pointers stay safely stored inside NonDereferencable objects. These casts
> +do not trigger `clang++ -fsanitize=vptr` errors.
> +
> +There is one escape hatch for expert to `unwrap` the typed pointer, it probably

Nit: "experts" or "an expert".

::: commit-message-a1fb8:14
(Diff revision 1)
> +but pointers stay safely stored inside NonDereferencable objects. These casts
> +do not trigger `clang++ -fsanitize=vptr` errors.
> +
> +There is one escape hatch for expert to `unwrap` the typed pointer, it probably
> +should never be used, but at least it should be more visible than someone
> +sneakily cast the pointer value to another type!

Nit: "casting".

::: mfbt/NonDereferencable.h:79
(Diff revision 1)
> +  // leave the ability to the user to create a NonDereferencable from any
> +  // pointer. Also, strictly speaking, in a constructor or destructor, `this`
> +  // points at an object still being constructed or already partially
> +  // destructed, which some very sensitive sanitizers could complain about.
> +  NO_POINTEE_CHECKS
> +  MOZ_IMPLICIT NonDereferencable(T* aPtr)

My sense is that we want this to be `explicit`, so we can make it obvious at call sites that something unusual is happening.  And it already looks as though your other patch explicitly constructs instances, anyway, so we might as well formalize it.

::: mfbt/NonDereferencable.h:92
(Diff revision 1)
> +    return *this;
> +  }
> +
> +  // Construct/assign from a compatible pointer type.
> +  template<typename U>
> +  NO_POINTEE_CHECKS MOZ_IMPLICIT NonDereferencable(U* aOther)

Same here on `explicit`.

::: mfbt/NonDereferencable.h:127
(Diff revision 1)
> +  // Extract the pointer value, untyped, but with the same constness as T.
> +  // Safe to use as a pure pointer value for e.g. logging purposes.
> +  // Please do not cast this value back to T*, use unwrap() instead to make it
> +  // more obvious.

So, off-the-wall idea, WDYT about making this return `uintptr_t`?  Such a return type would satisfy the ability to use this for logging, but would also discourage people from using the return value as an actual pointer: I think a `reinterpret_cast` at the call site of `value()` would make a reviewer somewhat more suspicious than a `static_cast`.

::: mfbt/NonDereferencable.h:135
(Diff revision 1)
> +  // Unwrap the fully-typed raw pointer, assuming the pointed-at object is now
> +  // alive.
> +  // Use with extreme caution, as that pointer won't prevent dereferencing!
> +  T* unwrap() const

Unless I am mistaken, it looks like the accompanying patch doesn't actually use this function?  In which case I think we should remove it until such time as somebody requires it.  People can get the same functionality with `value()` if they *really* need it.

::: mfbt/tests/TestNonDereferencable.cpp:148
(Diff revision 1)
> +{
> +  // Convert `this` from `CRTPBase*` to `T*` while construction is still in
> +  // progress; normally UBSan -fsanitize=vptr would catch this, but using
> +  // NonDereferencable should keep UBSan happy.
> +  CRTPBase()
> +    : mDerived(this)

We have compiler warnings about the use of `this` in constructors; does this construction trigger those warnings?  Maybe such a warning only applies when `this` has virtual member functions.
Attachment #8964746 - Flags: review?(nfroyd) → review-

Comment 9

a year ago
mozreview-review
Comment on attachment 8964747 [details]
Bug 1448494 - Use NonDereferenceable in DDLogger -

https://reviewboard.mozilla.org/r/233472/#review240616

r=me with the change below, which will be required by the previous patch.

::: commit-message-40912:1
(Diff revision 1)
> +Bug 1448494 - Use NonDereferencable in DDLogger - r?froydnj

Nit: spelling of the type, here and throughout.
Attachment #8964747 - Flags: review?(nfroyd) → review+
Assignee

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8964746 [details]
Bug 1448494 - NonDereferenceable<T> wraps a T* and prevents dereferencing ops -

https://reviewboard.mozilla.org/r/233470/#review240606

Thank you for this initial review.
It may look like a lot, but I don't think there's that much: A value type with a couple of accessors, and static_cast helpers, just what I needed. :-)

> Nit: I think this is supposed to be `NonDereferenceable`, see e.g. [wiktionary](https://en.wiktionary.org/wiki/dereferencable).  Here and throughout the patch.

Weird, my initial research pushed be towards the no-'e' version, based on 'referencable' being acceptable ( https://en.wiktionary.org/wiki/referencable ). Will change -- note that file names will change too, so mozreview might get confused!

> Nit: this is sort of weird word wrapping.  Move "misuses" to the previous line?

It makes the line 81 characters long. I suppose I can suffer this one in silence. :-)

> Nit: "casting".

How embarrassing.

I mean, I was totally testing your reviewer skills. Yes, that's exactly what happened.

> My sense is that we want this to be `explicit`, so we can make it obvious at call sites that something unusual is happening.  And it already looks as though your other patch explicitly constructs instances, anyway, so we might as well formalize it.

...

> Same here on `explicit`.

I thought that implicit was appropriate here, because if a function takes a NonDereferenceable, the caller may not really care about that, and provide a raw pointer. We're not really losing or transforming information, we're just automatically packaging it into a NonDerefenceable wrapper.

But I'm fine with `explicit`, it may indeed make it more obvious earlier that something "interesting" is happening, for a small cost in extra characters -- I don't mind meaningful verbosity. Thanks for the suggestion.

> So, off-the-wall idea, WDYT about making this return `uintptr_t`?  Such a return type would satisfy the ability to use this for logging, but would also discourage people from using the return value as an actual pointer: I think a `reinterpret_cast` at the call site of `value()` would make a reviewer somewhat more suspicious than a `static_cast`.

I'm always hesitant to use uintptr_t because it's only guaranteed to be able to store a pointer, it could theoretically be larger than needed! Though I doubt this would happen much in practice (?)
But I agree that it would reduce the chances of bad behavior.

The main downside is that DDLogger uses void* everywhere (because of my distrust for uintptr_t!), and I would need that suspicious reinterpret_cast or I would need to change a lot of things in DDLogger. :-(

How about I change value() as suggested, start using reinterpret_cast in the next patch (so we can land this thing), and I'll open a bug to switch to uintptr_t everywhere in DDLogger?
I'll add a 3rd patch to demonstrate this change, so you can judge if that's better overall; I'll fold that new patch into the other ones if you're happy with it.

> Unless I am mistaken, it looks like the accompanying patch doesn't actually use this function?  In which case I think we should remove it until such time as somebody requires it.  People can get the same functionality with `value()` if they *really* need it.

It's true I don't use it, and hopefully nobody ever will!

But I wanted to actively preempt misuses by providing this scary function now, instead of someone suddenly requiring it and crafting their own sneaky workaround elsewhere out of our view.
At least we control this one here, and it's intentionally forcing UBSan to do its checks.

So I really think we should keep it.
Possible alternatives:
- Add the method declaration but keep its definition commented-out, so if someone needs it one day, it will be ready to go.
- Make it private, it could be made public if needed; or add the appropriate `friend`s.

> We have compiler warnings about the use of `this` in constructors; does this construction trigger those warnings?  Maybe such a warning only applies when `this` has virtual member functions.

I found MSVC's C4355, "'this' : used in base member initializer list". But it doesn't apply here, as we're using `this` to initialize a member variable.

Are you thinking of another one?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
mozreview-review
Comment on attachment 8966858 [details]
Bug 1448494 - store/expose uintptr_t -- WIP I will roll this into the other patches if accepted -

https://reviewboard.mozilla.org/r/235534/#review241554

I guess this is maybe not as good as I thought.  Some comments below, though.

::: dom/media/doctor/DecoderDoctorLogger.h:294
(Diff revision 1)
>    static void LogConstruction(NonDereferenceable<const Subject> aSubject)
>    {
>      using Traits = DDLoggedTypeTraits<Subject>;
>      if (!Traits::HasBase::value) {
>        Log(DDLoggedTypeTraits<Subject>::Name(),
> -          aSubject.value(),
> +          reinterpret_cast<const void*>(aSubject.value()),

I had assumed the logger could propagate the integers all the way down to the actual `printf`-ish call.  Now that I look more closely at the code, that might not be possible, or it would involve a fair amount of code duplication.

::: mfbt/NonDereferenceable.h:145
(Diff revision 1)
> -    // at this time.
> +    return reinterpret_cast<T*>(value());
> -    return static_cast<T*>(value());
>    }
>  
>  private:
> -  T* MOZ_NON_OWNING_REF mPtr;
> +  uintptr_t mPtr;

I should have been clearer here!  My assumption was that `NonDereferenceable` would store pointers everywhere, and it'd only be at `value()` that we'd convert them to integers.  That would at least eliminate a good bit of stuff in the implementation above.

::: mfbt/tests/TestNonDereferenceable.cpp:32
(Diff revision 1)
>    int i2 = 2;
>  
>    // Construction with pointer.
>    NonDereferenceable<int> nd1(&i);
>    CHECK(!!nd1);
> -  CHECK(nd1.value() == static_cast<void*>(&i));
> +  CHECK(nd1.value() == reinterpret_cast<uintptr_t>(&i));

Having integers instead of pointers makes the tests somewhat ugly, though.
Attachment #8966858 - Flags: review?(nfroyd)

Comment 15

a year ago
mozreview-review
Comment on attachment 8964746 [details]
Bug 1448494 - NonDereferenceable<T> wraps a T* and prevents dereferencing ops -

https://reviewboard.mozilla.org/r/233470/#review241594

r=me with the `unwrap` change.

::: mfbt/NonDereferenceable.h:135
(Diff revision 2)
> +  // Unwrap the fully-typed raw pointer, assuming the pointed-at object is now
> +  // alive.
> +  // Use with extreme caution, as that pointer won't prevent dereferencing!
> +  T* unwrap() const

I still think we should remove `unwrap`.  If in a year or so, an analysis of `value()` shows that it's being used in sneaky ways, we could revisit this.  But honestly, given that this is going to be (primarily) used to silence analysis tools when we've demonstrated that they're throwing up false positives, I'm not too worried about misuse.
Attachment #8964746 - Flags: review?(nfroyd) → review+
For avoidance of doubt, I don't think part 3 should land as is.  So the other two parts are clear to land.
Assignee

Comment 17

a year ago
mozreview-review-reply
Comment on attachment 8966858 [details]
Bug 1448494 - store/expose uintptr_t -- WIP I will roll this into the other patches if accepted -

https://reviewboard.mozilla.org/r/235534/#review241554

Sorry for the delay in responding; low priority here! (So feel free to delay your own further responses if you're busy too.)

Providing uintptr_t from value() seems like a good idea, to make it harder (but not impossible) to misuse NonDereferenceable.

> I had assumed the logger could propagate the integers all the way down to the actual `printf`-ish call.  Now that I look more closely at the code, that might not be possible, or it would involve a fair amount of code duplication.

I will open a follow-up bug, I think DDLogger should use NonDereferenceable everywhere, it will be a larger change (but not complex).

> I should have been clearer here!  My assumption was that `NonDereferenceable` would store pointers everywhere, and it'd only be at `value()` that we'd convert them to integers.  That would at least eliminate a good bit of stuff in the implementation above.

I guess I like extremes :-)
Ok, only value() will produce a uintptr_t.

> Having integers instead of pointers makes the tests somewhat ugly, though.

Unfortunate necessity, for the added safety of NonDereferenceable::value().
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 21

a year ago
Forgot to remove unwrap(), will do so now...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

a year ago
mozreview-review
Comment on attachment 8966858 [details]
Bug 1448494 - store/expose uintptr_t -- WIP I will roll this into the other patches if accepted -

https://reviewboard.mozilla.org/r/235534/#review247678

Hm, I don't know if forcing use of `reinterpret_cast` is a good thing or not.  But let's go with this; I don't expect the class to be used widely, and we can revisit at a later point if need be.

Thanks for following up on this.
Attachment #8966858 - Flags: review?(nfroyd) → review+
Assignee

Comment 26

a year ago
mozreview-review-reply
Comment on attachment 8966858 [details]
Bug 1448494 - store/expose uintptr_t -- WIP I will roll this into the other patches if accepted -

https://reviewboard.mozilla.org/r/235534/#review247678

Thanks for the reviews.

I think reinterpret_cast in tests is acceptable, as we're testing the contained value against a raw pointer, it won't be a normal scenario.
And I'll open a bug against DDLogger to remove the reinterpret_cast and just use NonDereferenceable throughout.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8966858 - Attachment is obsolete: true

Comment 29

a year ago
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d97b8efcbc04
NonDereferenceable<T> wraps a T* and prevents dereferencing ops - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/51d020e4c2cb
Use NonDereferenceable in DDLogger - r=froydnj

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d97b8efcbc04
https://hg.mozilla.org/mozilla-central/rev/51d020e4c2cb
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee

Comment 31

a year ago
Updated title, as we didn't touch mfbt/Attributes.h in the end.
Summary: Add clang's no_sanitize("vptr") to mfbt/Attributes → Add mfbt pointer wrapper to manage clang's no_sanitize("vptr")
Assignee

Updated

a year ago
Blocks: 1459462
You need to log in before you can comment on or make changes to this bug.