vector::at() and map::at()

[Bruce Dawson]

While looking at storage\browser\blob\blob_reader.cc I noticed that it uses vector::at() six times. In some of those cases (BlobReader::ReadItem) the validity of the index is previously checked. In other cases it is not obviously explicitly checked but presumably the validity is guaranteed in some manner.

Regardless, I would have expected vector::at() to be banned because it throws an exception on an out-of-range index and exceptions are banned. A quick search of Chromium suggests that there are thousands of uses of vector::at(), map::at(), and related functions, with perhaps half of them being in tests.

Is this allowed? Is this advisable? Do we have any tests to ensure that raising an exception will cleanly terminate the program and record a crash dump?

[Peter Kasting]

On Thu, Sep 6, 2018 at 1:50 PM Bruce Dawson <bruce...@chromium.org> wrote:

Regardless, I would have expected vector::at() to be banned because it throws an exception on an out-of-range index and exceptions are banned. A quick search of Chromium suggests that there are thousands of uses of vector::at(), map::at(), and related functions, with perhaps half of them being in tests.

Is this allowed? Is this advisable? Do we have any tests to ensure that raising an exception will cleanly terminate the program and record a crash dump?

When I see at() in a CL I ask for it to be changed to [].  Usually the author was not trying to explicitly range-check but just write something less awkward than e.g. (*foo)[index].

I would say we should explicitly discourage at().  I don't know the answer to your last question.

PK

[Chris Palmer]

I've got my doubts about this part:

"In other cases it is not obviously explicitly checked but presumably the validity is guaranteed in some manner."

I don't think we should presume that. :)

It is becoming the case (twice now; hopefully no more) that we have to let people run complex parsers, implemented in C++, run in high-privilege processes (the browser). Thus memory corruption there is likely, and is a Critical, Pri-0 vulnerability. To cope with that, we (Platform Security Team) are working on hardening those parsers, which tends to involve adding CHECKs in places like base/containers. (We've found some bugs this way.)

Therefore, given my druthers, I'd rather have both operator[] and at be the same, and to have them CHECK or otherwise crash safely in the most efficient way (whatever that may be).

I don't know what the best way to achieve that in the STL is.

[JF Bastien]

Is performance a concern for only using operator[] ?

An actual compiler bug where a redundant check isn't being eliminated sounds like something we'd want to fix.

I don't know what the best way to achieve that in the STL is.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAOuvq22CePM4%2BfzujGrFxzihYtFw%2B7cB0kD0aU6MaLd8ccCfuw%40mail.gmail.com.

[Bruce Dawson]

In the case of BlobReader::ReadItem() the two range checks are separated by a call to ComputeBytesToRead() which makes it harder for the compiler to prove that the second check is redundant.

I guess it mostly seemed odd to me that we would be using exceptions to report errors rather than CHECK or DCHECK. I was initially concerned that crash might be missing these but a quick check (https://crash.corp.google.com/9da85347504a421c) shows that they are being caught and reported. If vector::at is approved then I guess there's nothing to do in general. If we want to avoid it we could supply something like this free function for const vector access, with all the appropriate variants:

template <typename T> const T& GetElement(const std::vector<T>& v, size_t index) {

  if (index >= v.size())

    CHECK(false);

  return v[index];

}

[JF Bastien]

On Thu, Sep 6, 2018 at 4:25 PM Bruce Dawson <bruce...@chromium.org> wrote:

In the case of BlobReader::ReadItem() the two range checks are separated by a call to ComputeBytesToRead() which makes it harder for the compiler to prove that the second check is redundant.

I'm sure friendly compiler people would be happy to optimize this.

 

I guess it mostly seemed odd to me that we would be using exceptions to report errors rather than CHECK or DCHECK. I was initially concerned that crash might be missing these but a quick check (https://crash.corp.google.com/9da85347504a421c) shows that they are being caught and reported. If vector::at is approved then I guess there's nothing to do in general. If we want to avoid it we could supply something like this free function for const vector access, with all the appropriate variants:

Both libstdc++ and libc++ should emit a trap when exceptions are disabled and they would have thrown.

[Greg Thompson]

On Thu, Sep 6, 2018 at 10:50 PM Bruce Dawson <bruce...@chromium.org> wrote:

While looking at storage\browser\blob\blob_reader.cc I noticed that it uses vector::at() six times. In some of those cases (BlobReader::ReadItem) the validity of the index is previously checked. In other cases it is not obviously explicitly checked but presumably the validity is guaranteed in some manner.

Regardless, I would have expected vector::at() to be banned because it throws an exception on an out-of-range index and exceptions are banned.

My interpretation of https://google.github.io/styleguide/cppguide.html#Exceptions is that vector::at() is implicitly banned because its only feature over operator[] is to throw an exception. I always ask authors to remove it when reviewing code.

 

A quick search of Chromium suggests that there are thousands of uses of vector::at(), map::at(), and related functions, with perhaps half of them being in tests.

Is this allowed? Is this advisable? Do we have any tests to ensure that raising an exception will cleanly terminate the program and record a crash dump?

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAE5mQiOuVVWMOFTAnTz8hbP-NkE0UqKpec3zT8Y4vJVS3UvNMw%40mail.gmail.com.

[Ian Clelland]

On Tue, Oct 16, 2018 at 5:16 AM Greg Thompson <g...@chromium.org> wrote:

On Thu, Sep 6, 2018 at 10:50 PM Bruce Dawson <bruce...@chromium.org> wrote:

While looking at storage\browser\blob\blob_reader.cc I noticed that it uses vector::at() six times. In some of those cases (BlobReader::ReadItem) the validity of the index is previously checked. In other cases it is not obviously explicitly checked but presumably the validity is guaranteed in some manner.

Regardless, I would have expected vector::at() to be banned because it throws an exception on an out-of-range index and exceptions are banned.

My interpretation of https://google.github.io/styleguide/cppguide.html#Exceptions is that vector::at() is implicitly banned because its only feature over operator[] is to throw an exception. I always ask authors to remove it when reviewing code.

 

A quick search of Chromium suggests that there are thousands of uses of vector::at(), map::at(), and related functions, with perhaps half of them being in tests.

Note, too, that map::at() can be used on const map objects, while map::operator[] cannot (it returns a non-const ref, and will insert items into the map if required).

 

Is this allowed? Is this advisable? Do we have any tests to ensure that raising an exception will cleanly terminate the program and record a crash dump?

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAE5mQiOuVVWMOFTAnTz8hbP-NkE0UqKpec3zT8Y4vJVS3UvNMw%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKQnr0Q2jCdwhBcU4BGO10Qw%2BvUMCWABQaW3EoD2g4YVi%3DFwtA%40mail.gmail.com.

[Peter Kasting]

On Tue, Oct 16, 2018 at 9:30 AM Ian Clelland <icle...@chromium.org> wrote:

On Tue, Oct 16, 2018 at 5:16 AM Greg Thompson <g...@chromium.org> wrote:

On Thu, Sep 6, 2018 at 10:50 PM Bruce Dawson <bruce...@chromium.org> wrote:

While looking at storage\browser\blob\blob_reader.cc I noticed that it uses vector::at() six times. In some of those cases (BlobReader::ReadItem) the validity of the index is previously checked. In other cases it is not obviously explicitly checked but presumably the validity is guaranteed in some manner.

Regardless, I would have expected vector::at() to be banned because it throws an exception on an out-of-range index and exceptions are banned.

My interpretation of https://google.github.io/styleguide/cppguide.html#Exceptions is that vector::at() is implicitly banned because its only feature over operator[] is to throw an exception. I always ask authors to remove it when reviewing code.

 

A quick search of Chromium suggests that there are thousands of uses of vector::at(), map::at(), and related functions, with perhaps half of them being in tests.

Note, too, that map::at() can be used on const map objects, while map::operator[] cannot (it returns a non-const ref, and will insert items into the map if required).

I use find() for pure lookup, but it does read more awkwardly than [].  I wish there was a const operator[] that returned a const ref.

PK