Closed Bug 1277377 Opened 5 years ago Closed 5 years ago

remove unsafe C-style cast in Vector

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: luke, Unassigned)

References

Details

Attachments

(1 file)

Attached 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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.