Re: crash upon startup in GWorkcenter, Recycler, ProjectCenter and Gorm

[Richard Frith-Macdonald]

> On 23 Dec 2017, at 05:19, Josh Freeman <gnustep_lists@twilightedge.com> wrote:
> 
> On Dec 22, 2017, at 10:05 AM, Richard Frith-Macdonald wrote:
> 
>> I think that's a possible indication of a bug in the compiler/runtime 
>> nonfragile API support;  it looks as if there may be an inconsistency in 
>> calculating the size of a structure used as an ivar.
> 
> 
> 
>   After more testing, here's some additional info that might be useful:
> 
> -- Hardware (virtual):
>   Clean, up-to-date Ubuntu 16.04 VM, 32bit
> 
> -- GNUstep install:   Used the current Ubuntu script from the wiki, with two 
> changes:
> - Removed the script's workaround for the ivar-offset issue (so it builds 
> using the current version of gnustep-make instead of an old version).
> - Built a version of gnustep-base from a couple days ago (Dec. 19) which 
> still used the obsolete -fobjc-nonfragile-abi build flag. (As previously 
> mentioned, the current version of base won't build with the Ubuntu script if 
> the script's ivar-offset workaround is removed).
> 
> -- Attachments:
>   Attached are 2 patches that print info about NSThread ivars at runtime, and 
> 2 debugging-session outputs that were run with patches installed. The patches 
> assume that the libs-base & libs-gui directories are in the same parent 
> directory as the libobjc2 directory (as they are when running the Ubuntu 
> script), because the patches add an #import "../../libobjc2/ivar.h" in order 
> to use the runtime's ivar & ivar_list types.
> 
>   The patches - one for base, and one for gui - each add a static C function 
> that prints out the objc-runtime's NSThread ivar layout, obtained using the 
> runtime API function, class_getIvarLayout(). The patch function also prints 
> the locations in memory of the current thread's instance members, obtained 
> using statements like: &thread->_gcontext.
> 
>   The base patch adds the diagnostic function to NSAutoreleasePool.m, & calls 
> it from -[NSAutorelease dealloc] before the NSThread's _autorelease_vars is 
> modified (which will leave a garbage value in _gcontext). The gui patch adds 
> a matching function to NSGraphicsContext.m & calls it from 
> -[NSGraphicsContext setCurrentContext:] before it frees the garbage value in 
> _gcontext & segfaults.
> 
>   The first output file, patched_base_session_output.txt, is the result of 
> installing only the base patch & running a debug session.
> 
>   The second output file, patched_base_and_gui_session_output.txt, is a 
> followup session after also installing the gui patch. In this session, 
> gnustep-base (previously patched) was not rebuilt, so base is the same binary 
> as the first session.
> 
> 
> -- Output summary:
>   In the first session (patched_base_session_output.txt), the diagnostic 
> output shows most of the NSThread members' addresses line up with the 
> runtime's ivar layout - up until the first member after the _autorelease_vars 
> struct, _gcontext.
> 
>   The runtime's ivar-layout offset for NSThread's _autorelease_vars member is 
> 44, and _gcontext's is 64. The 20-byte space for _autorelease_vars is the 
> smallest possible size for that struct on 32bit, as the struct is made up of 
> 5 members, each of which is 4 bytes (3 ints & 2 pointers - see struct 
> autorelease_thread_vars definition in NSAutoreleasePool.h).
> 
>   However, the memory address of thread->_gcontext is offset from the thread 
> instance's address by 56 - not the 64 listed by the runtime API - which is 
> only 12 bytes away from thread->_autorelease_vars. At that offset value, 
> _gcontext occupies the same memory location as the _autorelease_vars struct 
> member, pool_cache_size; When pool_cache_size is set in NSAutoreleasePool.m 
> (base), the same value will be accessed later in NSGraphicsContext.m (gui) as 
> _gcontext, causing a crash due to sending an objc message to a non-nil 
> garbage value.
> 
>   In the second session (patched_base_and_gui_session_output.txt), the 
> NSThread ivar diagnostics are also printed from within gui. Two notes:
> 
> 1) I had thought the "ivar-offset" issue was due to a mismatch between gui & 
> base ivar offsets (gui using a different offset to _gcontext than base), but 
> the output shows that this is not the case: gui & base actually match each 
> other's objc2-runtime ivar layouts and also have matching memory addresses 
> returned from &thread->member statements; The mismatch is between the 
> runtime's ivar layout and the memory addresses returned by &thread->member 
> statements.
> 
> 2) NSThread's ivar layout (obtained from the runtime API) was the same 
> between the first & second sessions. This was expected, because the only 
> change made between sessions was to add a static C function to 
> NSGraphicsContext.m, and a call to it from -[NSGraphicsContext 
> setCurrentContext:].
> 
>   Unexpectedly, the memory locations of NSThread members (as obtained from 
> &thread->member statements) did change between the two sessions, and not just 
> in gui, which was recompiled - it also changed in base, even though it was 
> the same binary in both sessions.
> 
>   The first session had memory address offsets (obtained from &thread->member 
> statements) for _autorelease_vars & _gcontext at 44 & 56, respectively; In 
> the second session, those members' offsets were 36 & 56 (_gcontext stayed in 
> the same place, _autorelease_vars shifted 8 bytes down, and now both members' 
> addresses disagreed with the runtime's ivar layout offsets of 44 & 64).
> 
>   The changed offsets also changed the location of the crash, because the in 
> the second session, _autorelease_vars & _gcontext were separated by 20 bytes 
> instead of 12 bytes, so they no longer overlapped. Subsequently, the call to 
> -[NSGraphicsContext setCurrentContext:] (where the first session crashed) 
> returned successfully, because _gcontext wasn't overwritten by a garbage 
> value when writing pool_cache_size. (The crash happened at a later point, in 
> -[GSWindowDecorationView layout]; Stack trace is in the output file).
> 
> 
>   Hope this helps track down the issue.

Thanks, that looks really interesting.  It adds two new things:
1. It tells us whether the problem is runtime or iompiler; the runtime is 
produicing the correct offset and the compiler is producing the wrong offset.
2. It tells us that the problem is that the offset used is 8 bytes too small 
(12 rather than 20)

I used your patch on my 64bit system ... 

BASE:
 class_getIvarLayout([NSThread class])
   _target : 8
   _arg : 16
   _selector : 24
   _name : 32
   _stackSize : 40
   _cancelled : 48
   _active : 49
   _finished : 50
   _exception_handler : 56
   _thread_dictionary : 64
   _autorelease_vars : 88
   _gcontext : 120
   _runLoopInfo : 128

 Current thread from GSCurrentThread(): 0xcfc158
   thread->_target: 0xcfc160 (8)
   thread->_arg: 0xcfc168 (16)
   thread->_selector: 0xcfc170 (24)
   thread->_name: 0xcfc178 (32)
   thread->_stackSize: 0xcfc180 (40)
   thread->_cancelled: 0xcfc188 (48)
   thread->_active: 0xcfc189 (49)
   thread->_finished: 0xcfc18a (50)
   thread->_exception_handler: 0xcfc190 (56)
   thread->_thread_dictionary: 0xcfc198 (64)
   thread->_autorelease_vars: 0xcfc1b0 (88)
   thread->_gcontext: 0xcfc1c0 (104)
   thread->_runLoopInfo: 0xcfc1d8 (128)

So the difference between the offsets in the runtime (correct) and the compiler 
(wrong) was 16 bytes, with the runtime thinking the strruct size was 32 and 
trhe compiler thinking it was 16;

It seems the compiler is sizing the structure as if it contained a pointer and 
two integers when it should actually be two pointers and three integers;

typedef struct autorelease_thread_vars
{
  __unsafe_unretained NSAutoreleasePool *current_pool;
  unsigned total_objects_count;
  __unsafe_unretained id *pool_cache;
  int pool_cache_size;
  int pool_cache_count;
} thread_vars_struct;