Should we have a presubmit to guard against &*managed_ptr ? |
||
Issue description
Until recently I widely wrote code like &*unique_ptr (as a short hand for unique_ptr.get()), mostly to fit into 80cols, thinking that they are absolutely identical. Apparently like me, other folks likes this pattern, see [1].
Recently I was doing the same in another project that uses UBSan, and I realized that &*unique_ptr seems to yield to undefined behaviour if the unique_ptr is nullptr.
This is the error I got from UBSan:
-----
runtime error: reference binding to null pointer of type 'perfetto::ipc::TestMessage'
../../src/ipc/deferred_unittest.cc:68: Failure
Expected: nullptr
Which is: NULL
To be equal to: &*msg
Which is: NULL
-----
Code causing this: [2]
I guess the error is not really specific to unique_ptr, but to anything that returns a reference.
If it's the case that this is UB, should we have a presubmit warning against it?
[1] https://cs.chromium.org/search/?q=%5C%26%5C*+f:%5C.(h%7Ccc)+-f:third_party/%5B%5EW%5D&sq=package:chromium&type=cs
[2] https://github.com/catapult-project/perfetto/commit/224526e827e9bed7026fe842f4d7cb31a5219b16#diff-433c968728ac46ea7f8a19330aedf745
,
Dec 13 2017
The problem here is not * alone, is the &* combination to get the pointer from something that is supposed to return a reference (which means that the pointer could be null). If I read this correctly &*(nullptr) won't necessarily return nullptr (or is just UBSan being too pessimistic just because it says *(nullptr) and doesn't realize that we are only trying to get the pointer after all) ? In other words, I understand that *(nullptr) is UB. My genuine question is: is &*(nullptr) also UB? That is the case for which I was proposing a warning (if it's the case).
,
Dec 13 2017
> My genuine question is: is &*(nullptr) also UB? Yes it is. > That is the case for which I was proposing a warning (if it's the case). It would be tricky though, because the warning would have to somehow know that the unique_ptr or whatever might be nullptr. And as dcheng said, generally when the dereference operator is used, it's assumed that the operand can be dereferenced.
,
Jan 10
Archiving P3s older than 1 year with no owner or component. |
||
►
Sign in to add a comment |
||
Comment 1 by dcheng@chromium.org
, Dec 13 2017