Page MenuHomePhabricator

[libc++][ranges] attempt to fix proxy iterator issues that cause Chromium to crash
ClosedPublic

Authored by var-const on Jul 20 2022, 4:12 PM.

Details

Summary

[libc++][ranges] attempt to fix proxy iterator issues that cause Chromium to crash

The crash reported by Chrome v8 is related to sorting with v8::internal::AtomicSlot
According to https://chromium.googlesource.com/v8/v8/+/9bcb5eb590643db0c1f688fea316c7f1f4786a3c/src/objects/slots-atomic-inl.h
AtomicSlot is indeed a proxy iterator with a proxy type AtomicSlot::Reference

https://reviews.llvm.org/D130197 correctly spotted issues in __iter_move, but the fix does not fix the issue.
The reason is that AtomicSlot::operator* returns a prvalue Reference. After the fix in D130197, the return type
of __iter_move is Reference&&. But the rvalue reference is bond to the temporary value returned by operator*,
which will be dangling after __iter_move returns.

The idea of the fix in this change is borrowed from C++17's move_iterator
https://timsong-cpp.github.io/cppwp/n4659/move.iterators#move.iterator-1
When underlying reference is a prvalue, we just return it by value.

Diff Detail

Event Timeline

huixie90 created this revision.Jul 20 2022, 4:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2022, 4:12 PM
Herald added a subscriber: mgrang. · View Herald Transcript
huixie90 requested review of this revision.Jul 20 2022, 4:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2022, 4:12 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
var-const accepted this revision.Jul 20 2022, 6:06 PM

LGTM. It looks like the new tests still pass even without the fix, which means we still have work to do there. However, the tests are a great starting point, and I manually verified that the fix works, so I think it's good to go.

This revision is now accepted and ready to land.Jul 20 2022, 6:06 PM
var-const commandeered this revision.Jul 20 2022, 6:13 PM
var-const edited reviewers, added: huixie90; removed: var-const.

I'll commandeer this just to land this quicker -- @huixie90 I hope that's okay with you.

This revision now requires review to proceed.Jul 20 2022, 6:13 PM
var-const updated this revision to Diff 446318.Jul 20 2022, 6:13 PM

Rebase and reword the description.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 20 2022, 6:16 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.