DyKnow / SignalR-ObjC

Objective-C Client for the SignalR Project works with iOS and Mac
MIT License
442 stars 213 forks source link

Crash during parsing socket message #303

Open daryaklochkova opened 4 years ago

daryaklochkova commented 4 years ago

Hi!

I have an issue with parsing a socket event message. My app crashes with error: Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[__NSCFString appendString:]: nil argument' in function - (void)add:(NSData *)buffer SRChunkBuffer.

The buffer argument seems to be corrupt. I am sure I received the correct information because other clients parse it correctly. Could you help me, please?

Thank you, Darya

joeldart commented 4 years ago

any chance this is signalr core? we dont have support for that yet. what version of signalr protocol are you using on the server?

daryaklochkova commented 4 years ago

We are using 2.3.0. Is it possible singnalr core if other clients are working fine? How can I check it?

joeldart commented 4 years ago

Signalr on dotnetcore is an entirely different protocol which is why I asked.

2.3 should be compatible though. what's the full stack trace and what data is being received?

daryaklochkova commented 4 years ago

I'm sorry, I made the mistake. The server uses asp net signalr(.net framework, not "core")

daryaklochkova commented 4 years ago

Stack trace after crash: CrashStackTrace.txt

Stack trace after OBJ-C exception breakpoint CrashStackTraceWithBreakpoint.txt

received data receivedMessage.txt

Thanks for the help

joeldart commented 4 years ago

If I am reading your message correctly, it is crashing because it is trying to insert nil into the buffer here: https://github.com/DyKnow/SignalR-ObjC/blob/feature-dev/SignalR.Client/Transports/ServerSentEvents/SRChunkBuffer.m#L48

- (void)add:(NSData *)buffer {
    [_buffer appendString:[[NSString alloc] initWithData:buffer encoding:NSUTF8StringEncoding]];
}

but that should not be nil because the calling method checked that it had a greater-than-zero length here: https://github.com/DyKnow/SignalR-ObjC/blob/feature-dev/SignalR.Client/Transports/ServerSentEvents/SREventSourceStreamReader.m#L97-L102

                NSInteger read = [buffer length];
                if(read > 0) {
                    // Put chunks in the buffer
                    _offset = _offset + read;

                    [_buffer add:buffer];
              ...

2 things:

  1. what version of signalr-objc are you using
  2. If you change the line in SRChunkBuffer.m to be
    - (void)add:(NSData *)buffer {
    if (!buffer) return;
    [_buffer appendString:[[NSString alloc] initWithData:buffer encoding:NSUTF8StringEncoding]];
    }

    does it resolve the issue?

daryaklochkova commented 4 years ago

Yes, you are right, buffer not nil. But [[NSString alloc] initWithData:buffer encoding:NSUTF8StringEncoding]] return nil. I guess the buffer was corrupted. I need this message so I cannot ignore it.

I am using 2.0 version

joeldart commented 4 years ago

version: 2.0.2 is the latest, just to be sure there's nothing upstream that's been fixed.

data: interesting, what data type is the data that is returned. Im not sure ive seen eventsource return anything but string data.

could you write those bytes out to a file or something? I'd like to see if we could reproduce this in a unit test.

daryaklochkova commented 4 years ago

I tried to upgrade the library. I change my pod file to pod 'SignalR-ObjC', '~> 2.0.2' and run pod install. But I don't know how to verify that the library update was successful. If the steps correct issue still reproduces on 2.0.2

SocketCrashReceivedData.txt I collect this data in https://github.com/DyKnow/SignalR-ObjC/blob/c412cc38618b27c83fe170eaa9da95aa0c7eb42a/SignalR.Client/Transports/ServerSentEvents/SREventSourceStreamReader.m#L90

NSData *buffer = [stream propertyForKey:NSStreamDataWrittenToMemoryStreamKey]; Is that what you asked for?

joeldart commented 4 years ago

The buffer for https://github.com/DyKnow/SignalR-ObjC/blob/c412cc38618b27c83fe170eaa9da95aa0c7eb42a/SignalR.Client/Transports/ServerSentEvents/SREventSourceStreamReader.m#L95

on the line right after is the one that will get passed into the crashing line.

daryaklochkova commented 4 years ago

About the data format, my issue is reproduced in 50% of cases. I can not find any difference between the messages that we can parse and can not. So I think the data format is not the reason

daryaklochkova commented 4 years ago

data for line 95 CrashBufferData.txt

joeldart commented 4 years ago

thanks - I cant hop on this right now. To be sure I understand, when you pass that into [[NSString alloc] initWithData:buffer encoding:NSUTF8StringEncoding]] you found it returns nil?

daryaklochkova commented 4 years ago

Yes, that's right

daryaklochkova commented 4 years ago

I investigated the issue a bit. I get multiple socket events at the same time, so I believe multithreading may be the cause of the issue.

Also, I tried to skip the data writing if the function [[NSString alloc] initWithData:buffer encoding:NSUTF8StringEncoding]] returns nil as you suggested to me. In this case, I can reproduce a crash in this place https://github.com/DyKnow/SignalR-ObjC/blob/c412cc38618b27c83fe170eaa9da95aa0c7eb42a/SignalR.Client/Transports/ServerSentEvents/SREventSourceStreamReader.m#L90 NSData *buffer = [stream propertyForKey:NSStreamDataWrittenToMemoryStreamKey]; Thread 1: EXC_BAD_ACCESS (code=1, address=0x1117b4000) Stack traces BadAccessStackTrace.txt

I hope it will be helpful

joeldart commented 4 years ago

This sounds similar to https://github.com/DyKnow/SignalR-ObjC/commit/3628a5d51feb2fd2e9e673df2a856a1a5404d672 which is still on an upstream branch https://github.com/DyKnow/SignalR-ObjC/compare/feature-closeOnTimeout

That's the version we have been using for over a year now, but I havent dont the work to review, merge, etc.

feature-closeOnTimeout

so you could try updating your podfile to pod 'SignalR-ObjC', :git => 'https://github.com/dyknow/SignalR-ObjC.git', :branch => 'feature-closeOnTimeout'

If that's the same issue, then it should avoid this behavior altogether.

daryaklochkova commented 4 years ago

Thank you. Unfortunately, both issues are still reproducing.