Re: Bug in base: NSMapGet()

[Daniel Ferreira (theiostream)]

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.
>