[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Bug in base: NSMapGet()
From: |
Daniel Ferreira (theiostream) |
Subject: |
Re: Bug in base: NSMapGet() |
Date: |
Sun, 4 Jun 2017 11:23:51 -0300 |
Sadly, given that objc_loadWeak()'s behavior is non-deterministic with
arbitrary pointers, it took me quite a while to reproduce the issue
(since it works most times somehow) and I only managed to do it with a
very specific combination of JavaScriptCore values, which would be a
pain for anyone to reproduce. I really couldn't generate a less
specific case.
The following code crashes under Base + libobjc2 but works under
Apple's Foundation + libobjc:
JSContext *ctx = [[JSContext alloc] init];
NSPointerFunctionsOptions keyOptions =
NSPointerFunctionsOpaqueMemory | NSPointerFunctionsOpaquePersonality;
NSPointerFunctionsOptions valueOptions =
NSPointerFunctionsWeakMemory | NSPointerFunctionsObjectPersonality;
NSMapTable *table = [[NSMapTable alloc]
initWithKeyOptions:keyOptions valueOptions:valueOptions capacity:0];
JSValueRef ref = JSValueMakeBoolean([ctx JSGlobalContextRef], YES);
JSValueRef ref2 = JSContextGetGlobalObject([ctx JSGlobalContextRef]);
JSValue *wrapper = [[JSValue alloc] initWithValue:ref inContext:ctx];
JSValue *wrapper2 = [[JSValue alloc] initWithValue:ref2 inContext:ctx];
NSMapInsert(table, ref2, wrapper2);
NSMapGet(table, ref);
As a side note, this *may* work using Base + non-libobjc2, because
objc_loadWeak(&ref2); crashes under libobjc2 and not under Apple's
libobjc. To me, though, this looks as consequence of some
implementation detail rather than something that enables us to call
objc_loadWeak() with non-objects.
-- Daniel.
On Sun, Jun 4, 2017 at 2:58 AM, Fred Kiefer <address@hidden> wrote:
> Thank you Daniel,
>
> I think that this time you found a real issue. It surely would help Richard
> to understand this better if you could provide your new test code. My
> expectation is that you just use an object as the value now.
> You should always try to make it as easy as possible for the project
> maintainer to reproduce your issue, even if already provide a fix. The best
> thing to do here is to write a test case that could be added to
> base/Tests/base/NSMapTable.
>
> Fred
>
>
>> Am 04.06.2017 um 07:26 schrieb Daniel Ferreira (theiostream)
>> <address@hidden>:
>>
>> Okay, I'm ashamed of how long and how many experiments it took me to
>> finally see this, and I'm sorry for the misleading example I sent on
>> the first thread e-mail that was supposed to go wrong anyway.
>>
>> The issue whose fault is Base's is that despite the key barriers being
>> set to (NSPointerFunctionsOpaqueMemory |
>> NSPointerFunctionsOpaquePersonality), i.e. with nothing to do with
>> ARC, Base's NSConcreteMapTable was still calling objc_loadWeak() to
>> get the key value.
>>
>> After looking at the wrong place for hours, I finally noticed that in
>> GSI_MAP_NODE_IS_EMPTY we were reading the table nodes' key values
>> using the GSI_MAP_READ_VALUE macro, which applies the *value* barriers
>> to whatever it's reading. This way, we were applying value barriers to
>> reading the keys, and since the value barriers used
>> NSPointerFunctionsWeakMemory, it ended up calling ARC functions on the
>> raw pointers that made up the keys causing all sorts of unexpected
>> behavior.
>>
>> Here's the diff of how I fixed it. (Since how we submit code
>> contributions is apparently unstable with the change to Git, I suppose
>> I will temporarily send it here for comments, since it's a small one
>> anyway.)
>>
>> diff --git a/Headers/GNUstepBase/GSIMap.h b/Headers/GNUstepBase/GSIMap.h
>> index 47fc8d3..b0ac494 100644
>> --- a/Headers/GNUstepBase/GSIMap.h
>> +++ b/Headers/GNUstepBase/GSIMap.h
>> @@ -157,9 +157,9 @@ extern "C" {
>> # define GSI_MAP_WRITE_VAL(M, addr, obj) (*(addr) = obj)
>> #endif
>> #if GSI_MAP_HAS_VALUE
>> -#define GSI_MAP_NODE_IS_EMPTY(M, node) (((GSI_MAP_READ_VALUE(M,
>> &node->key).addr) == 0) || ((GSI_MAP_READ_VALUE(M, &node->value).addr
>> == 0)))
>> +#define GSI_MAP_NODE_IS_EMPTY(M, node) (((GSI_MAP_READ_KEY(M,
>> &node->key).addr) == 0) || ((GSI_MAP_READ_VALUE(M, &node->value).addr
>> == 0)))
>> #else
>> -#define GSI_MAP_NODE_IS_EMPTY(M, node) (((GSI_MAP_READ_VALUE(M,
>> &node->key).addr) == 0))
>> +#define GSI_MAP_NODE_IS_EMPTY(M, node) (((GSI_MAP_READ_KEY(M,
>> &node->key).addr) == 0))
>> #endif
>>
>> -- Daniel.
>