Page MenuHomePhabricator

Memory corruption issure for DenseStringElementsAttr

Authored by rsuderman on Apr 24 2020, 7:42 PM.



There was a memory corruption issue where the lifespan of the ArrayRef<StringRef> would fail. Directly passing the data will avoid the issue.

Diff Detail

Event Timeline

rsuderman created this revision.Apr 24 2020, 7:42 PM
Herald added a project: Restricted Project. · View Herald Transcript
rriddle accepted this revision.Apr 24 2020, 8:00 PM
This revision is now accepted and ready to land.Apr 24 2020, 8:00 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Apr 25 2020, 9:31 PM

I don't understand the fix, can you elaborate? Seems like the ArrayRef is passed by value to the KeyTy so I don't get the lifetime issue?

mehdi_amini added inline comments.Apr 25 2020, 9:39 PM

We were creating an ArrayRef pointing to the local variable firstElt before, but since it is a reference it should be fine. So there is likely a copy of the StringRef in the implicit conversion to ArrayRef.

The change here is making us taking the hash of the entire array instead of the first element, I don't think this is necessary, something like this should work as well:

return KeyTy(ty, ArrayRef<StringRef>{&firstElt, 1}, hashVal, /*isSplat=*/true);
mehdi_amini added inline comments.Apr 26 2020, 12:26 PM

Actually nicer and equivalent I think:

return KeyTy(ty, data.take_front(), hashVal, /*isSplat=*/true);