1277377 - remove unsafe C-style cast in Vector

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Luke Wagner [:luke]]

Attachment: fix-vector

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.

Comment 1

[Dan Gohman [:sunfish]]

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 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a30acee45af
prevent unsafe C-style cast in Vector (r=waldo)

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a30acee45af