remove unsafe C-style cast in Vector

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: luke, Unassigned)

Tracking

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

Posted patch fix-vectorSplinter Review
This recently started to type-check when it clearly shouldn't:

  Vector<int*> v;
  v.append((float*)nullptr);

The reason is that is-pod specialization of new_ now contains:

  *aDst = T(Forward<Args>(aArgs)...);

which, when there is a single arg, means a raw C-style cast.

The fix in this patch is:

  T temp(Forward<Args>(aArgs)...);
  *aDst = temp;

I tried to fix with 'T{Forward<Args>(aArgs)...}' but apparently this comes with some much stricter rules (w.r.t narrowing) that we violate all over the place.  Maybe we want to opt into this in the future and add explicit casts to all those places, but we'd want to do that in the non-POD case too but that's a bigger patch and first I'd like to unregress safety.
Attachment #8758893 - Flags: review?(jwalden+bmo)
Summary: remove unsafe C-style cast → remove unsafe C-style cast in Vector
Comment on attachment 8758893 [details] [diff] [review]
fix-vector

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

This looks right to me. This only affects the POD case, so the extra copy should always be easy for compilers to optimize away.
Comment on attachment 8758893 [details] [diff] [review]
fix-vector

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

Stab all compilers that fail to optimize |void func(T& ref) { new (&ref) T(...); }| with a fork.
Attachment #8758893 - Flags: review?(jwalden+bmo) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a30acee45af
prevent unsafe C-style cast in Vector (r=waldo)
https://hg.mozilla.org/mozilla-central/rev/8a30acee45af
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.