SocketRocket icon indicating copy to clipboard operation
SocketRocket copied to clipboard

SRProxyConnect Crash

Open LoveLifeLoveSelf opened this issue 6 years ago • 5 comments

EXC_BAD_ACCESS (SIGBUS)

0 libobjc.A.dylib objc_msgSend + 16 1 xxxxxx -[SRProxyConnect _failWithError:] (SRProxyConnect.m:140) 2 xxxxxx -[SRProxyConnect stream:handleEvent:] (SRProxyConnect.m:0) 3 CoreFoundation _signalEventSync + 212 4 CoreFoundation _cfstream_shared_signalEventSync + 460 5 CoreFoundation CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION + 24 6 CoreFoundation __CFRunLoopDoSources0 + 276 7 CoreFoundation __CFRunLoopRun + 1204 8 CoreFoundation CFRunLoopRunSpecific + 552 9 Foundation -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 304 10 xxxxxx -[SRRunLoopThread main] (SRRunLoopThread.m:70) 11 Foundation NSThread__start + 1040 12 libsystem_pthread.dylib _pthread_body + 272 13 libsystem_pthread.dylib _pthread_body + 0

this crash found online

LoveLifeLoveSelf avatar Aug 27 '18 04:08 LoveLifeLoveSelf

@kommen @lukeredpath @xaviershay @danielrhammond @mk Ask for help,thanks! #

LoveLifeLoveSelf avatar Aug 27 '18 04:08 LoveLifeLoveSelf

The SRProxyConnect class is rather unsafe around NSStream lifetime and event dispatches. There have been similar issues in the past with SRWebSocket but those fixed haven't made it to SRProxyConnect.

The input and output streams must both be scheduled on the SR_networkRunLoop run loop (currently only the inputstream is, with a commented out line for the output stream which has been there since the class was created). In addition, the closing, nil-ing out of instance variables, and nil-ing out of the delegates for those streams must be performed on that same runloop, otherwise the streams themselves can end up getting deallocated while events are being processed, or the delegate (SRProxyConnect itself) can end up getting deallocated while events are being processed, since the streams keep a non-retained reference to it.

So, although I haven't got round to submitting my fix for this:

  • Schedule the output stream on SR_networkRunLoop similar to how the inputStream already is, by uncommenting the scheduleInRunLoop call. Also add a call to unschedule in the appropriate place (search for the input stream one).
  • In all the code that clears the streams' delegates, unschedules them, closes them, nils them out etc. (including deinit), this has to be dispatched synchronously to SR_networkRunLoop to be actioned, to ensure there can be no events being processed concurrently and that after clearing the streams' delegate SRProxyConnect itself can happily die and turn into trash memory without any more queued or still-running events getting processed. You can execute something on the correct thread/runloop with:
    // Note: if we are already on the correct thread, this will execute inline
    [self performSelector:@selector(tearDownStreams:)
                 onThread:[SRRunLoopThread sharedThread]
               withObject:nil
            waitUntilDone:YES];
    

MikeWeller avatar Nov 28 '18 13:11 MikeWeller

Pasting some raw notes from a crash analysis I did:

SRProxyConnect crashes

Two main variants of the crash we see:

BUS_ADRALN at 0xffffffff89abcdef we're trying to jump to this bad address from failWithError:, which comes from the value of the _completion instance variable from an earlier call to openNetworkStreamWithCompletion. So the completion is bad, so the memory of the object is bad. The delegate 'self' in this case is 0x00000001d40eba80 which looks like it could be a valid pointer value, but maybe dangling/deallocated/reallocated/etc.

SEGV_ACCERR at 0xd000000010d449a8 this pointer is from the x0 register which is the 'self' value itself in a call to -[SRProxyConnect stream:handleEvent:], pointer itself looks bad which would suggest the calling stream itself has been deallocated and its delegate property/ivar gets trashed.

So SRProxyConnect is having its stream:handleEvent: called by the stream, since it sets itself as a delegate, but it looks like the self delegate pointer value itself is invalid which suggests it is the stream itself that has been deallocated, its memory trashed and it ends up dispatching a delegate call to a runloop with a bad pointer. Sometimes the pointer is semi-valid or dangling and we get further along but try calling a bad _completion on SRProxyConnect. Sometimes the pointer is entirely bad and we get an immediate SEGV_ACCERR when we try and load any instance data from it.

The event code switch is optimized/merged for the two NSStreamEventEndEncountered and NSStreamEventErrorOccurred cases so we don't have an exact source location in the symbolicated crash, but the event code variable itself comes from register x3 one frame up and isn't mutated, and has a value of 0x0000000000000008 (we've lost it in the BYS_ADRALN one apparently), which is NSStreamEventErrorOccurred = 1UL << 3

The streamError that is pulled from aStream ends up in register x2 in the crashing frame, if it's null we reassign by creating a new one (SRHTTPErrorWithCodeDescription(500, 2132,@"Proxy Error");), but it has a value of x2: 0x0000000000007803 which suggests it was bad when we got it from the stream itself.

So everything points to the stream itself being bad with bad data members, calling us (the delegate) via its bad/dangling delegate pointer, as well as its other members like the stream error being invalid.

MikeWeller avatar Nov 28 '18 13:11 MikeWeller

To further clarify what I think is happening:

  • An error occurs on the input stream
  • That error is processed on the SR_networkRunLoop via failWithError and we end up throwing away the input stream and output streams by assigning them to nil.
  • The output stream ends up getting deallocated as a result, but it still tries to dispatch an error event to its own delegate on some other thread using its now trashed data members.
  • The stream documentation says "You should never attempt to access a scheduled stream from a thread different than the one owning the stream’s run loop." which apparently includes destroying that stream.
  • The fix is to serialize everything onto the same SR_networkRunLoop to ensure the output stream is destroyed and unscheduled etc. from that run loop and no more events can end up getting dispatched after it has been closed/destroyed etc.

MikeWeller avatar Nov 28 '18 13:11 MikeWeller

So is there a conclusion for this? I received more and more this crash for my app these days.

codingingBoy avatar Oct 10 '22 12:10 codingingBoy