Page MenuHomePhabricator

[libomptarget] Avoid double-frees and user-after-frees of entries
Needs ReviewPublic

Authored by gValarini on Aug 25 2022, 10:36 AM.

Details

Reviewers
jdoerfert
ye-luo
Summary

The current mechanism using TIDs to delegate a map entry
ownership/deletion-responsibility during data ends can lead to
double-free and user-after-free problems. By tracking the threads that
uses the same entry on the targetDataEnd, we can delay the entry
deletion to be done by the last thread that leaves the targetDataEnd,
ensuring no double-free/user-after-free problems may occur.

Diff Detail

Event Timeline

gValarini created this revision.Aug 25 2022, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 10:36 AM
gValarini requested review of this revision.Aug 25 2022, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2022, 10:36 AM
jdoerfert added inline comments.Sep 7 2022, 10:34 AM
openmp/libomptarget/include/device.h
76

Make a separate header, this is reusable.

openmp/libomptarget/src/omptarget.cpp
687

I think, this can leave an entry intact on the device:

T1: arrives here, TPR is still life so DeleterThreadCount is 3, won't destory the entry, HDTTMap is released.
T2: arrives here, TPR is still life so DeleterThreadCount is 3, won't destory the entry. HDTTMap is released.
Both exit this function, TPRs are destroyed, DeleterThreadCount is decremented twice and reaches 1, but nobody executing the code that would delete the entry.
ye-luo added inline comments.Sep 7 2022, 10:49 AM
openmp/libomptarget/src/omptarget.cpp
687

Can this be protected by not calling HDTTMap.destroy()?

ye-luo added inline comments.Sep 7 2022, 12:17 PM
openmp/libomptarget/src/omptarget.cpp
687

Second thought on this. If the mutex protection from HDTTMap is anyway needed, std::atomic seems likely not needed.
Probably need to revisit D123443, D123444 and D123446.