Bug #2865
closed
Possible false remove in Face::cancelPendingInterest (address used as handle)
100%
Description
expressInterest returns the memory address of the interest copy as the PendingInterestId.
And removePendingInterest uses this memory address to find the entry to remove it from the PIT.
Note that removePendingInterest is supposed to do nothing if the interest is no longer in the PIT.
Therefore, the following failure mode is possible:
- The application calls expressInterest for interest 1 and receives memory address X of the interest copy as the PendingInterestId.
- The application keeps X.
- A data packet is received, the library removes interest 1 from the PIT, freeing the memory of the interest.
- The application calls expressInterest again for interest 2. (This returned PendingInterestId is ignored.) The library just happens to use the same memory address X for the interest copy.
- Another part of the application wants to cancel the original interest 1, and calls removePendingInterest(X).
- Interest 1 is no longer in the PIT. But the memory address X is re-used as the same PendingInterestId for interest 2, so the library falsely removes interest 2 from the PIT.
Updated by Alex Afanasyev over 9 years ago
I understand the issue, but don't really want to do anything about it. Chance that this would happen is small and chance that this would negatively affect application in a major way is even smaller.
Updated by Anonymous over 9 years ago
If nobody else has any discussion, we can abandon this.
Updated by Junxiao Shi over 9 years ago
- Description updated (diff)
- Category set to Base
- Target version set to Unsupported
This is a valid Bug report and should not be abandoned. I have marked TargetVersion=Unsupported to indicate that there's no recent plan to fix this problem.
Updated by Davide Pesavento over 5 years ago
- Subject changed from Possible false remove in removePendingInterest (address used as ID) to Possible false remove in Face::cancelPendingInterest (address used as handle)
- Target version changed from Unsupported to v0.7
- Start date deleted (
06/09/2015)
The described sequence of events has become a lot more likely with the introduction of ScopedPendingInterestHandle (#4316).
One of its most common usage patterns is assigning the result of expressInterest to a class member field of type ScopedPendingInterestHandle when re-expressing an Interest that has timed out or has been nacked. As Jeff said, expressInterest may allocate a copy of the new Interest at the same memory address[1] as the timed-out/nacked Interest. If that happens, when expressInterest returns and its result is assigned to the handle, the move assignment operator will try to cancel the old (timed-out) Interest but it will in fact end up canceling the new (just re-expressed) Interest.
[1] In some circumstances, if the pattern of memory (de)allocations repeats itself over and over, the memory addresses of newly allocated objects are very predictable (same address >90% of the time in my tests).
Updated by Junxiao Shi over 5 years ago
- Status changed from New to In Progress
- Assignee set to Junxiao Shi
- Estimated time set to 3.00 h
Updated by Junxiao Shi over 5 years ago
- Status changed from In Progress to Code review
- % Done changed from 0 to 80
https://gerrit.named-data.net/5233 assigns PendingInterestId from an atomic counter to avoid duplicates.
Updated by Junxiao Shi over 5 years ago
The same bug also occurs on InterestFilterId and RegisteredPrefixId. Since they aren't causing test failures now, I'll fix them in #3919.
we should eventually (after we've removed PendingInterestId from the public API) switch to
using PendingInterestId = std::uintptr_t;and change all "const PendingInterestId*" to just "PendingInterestId"
Yes, and probably more than that.
Updated by Junxiao Shi over 5 years ago
- Status changed from Code review to Closed
- % Done changed from 80 to 100