[webkit-dev] [webkit-reviewers] usage of auto

[Michael Catanzaro]

Perhaps we need to be especially careful about replacing Refs and
RefPtrs with auto. It was mentioned that using typedefs to hide pointer
types is often the source of serious bugs. A similar rule could apply
to using auto with smart pointers.

On Wed, 2017-01-11 at 09:43 -0800, Darin Adler wrote:
> > On Jan 11, 2017, at 9:41 AM, Alexey Proskuryakov <ap at webkit.org>
> > wrote:
> > 
> > In a way, these are read-time assertions.
> 
> Exactly. A type name is a read-time assertion of the specific type
> that a variable has and “auto” is a read-time assertion that the type
> of the variable is the same as the type of the expression on the
> right.
> 
> — Darin

That is a useful observation. Consider this simple scenario:

int i = returnsSomeIntType();
auto i = returnsSomeIntType();

Sometimes you want a variable that is the same type as the expression
on the right, and you are worried about possible bugs if the type of
the expression on the right changes. In my experience, this is the
typical case. Then auto is probably most appropriate. For instance, if
the function returns an int but in the future is changed to return an
int64_t, then you will be very happy that you chose to use auto, as it
avoids a truncation bug that would have been extremely difficult to
discover through code inspection. We face exactly this problem, for
example, in bug #165790, where the goal is to change the type of a
function from unsigned to size_t, but the function is named "length" so
it's virtually impossible to grep for call sites to update. (On the
other hand, you really always have to check call sites in such a
situation, because we have not always used auto.)

But sometimes the type of the expression on the right is not so
important. For instance, I reviewed a patch recently where I discovered
a very subtle integer underflow bug. It was not caused by auto, so it's
not a perfect example, but in this case it was important to ensure
that, regardless of the type returned from the function, it needed to
be assigned to a variable of type int64_t to avoid underflow later on.
In this case, auto was an inferior option, as the only way to safely
use auto would have been this:

auto i = static_cast<int64_t>(returnsSomeIntType());

which is surely inferior to just not using auto:

int64_t i = returnsSomeIntType();

Michael

P.S.

Not very related: the underflow in question was of the form

int64 result = a - b()

where a was a size_t, and b() returned a size_t, and we wanted to
receive a negative result if b() was greater than a. This is such an
easy mistake to make and very difficult to spot. :/ It's worth a little
paranoia to consider that an attacker might try to intentionally insert
such a vulnerability.