Crash in tools.cpp line 90: trimspaces->righttrim()

[xaphod]

Hi there, Thanks for upnpx, it's really useful. I wonder if you can fix this bug for me... it's a crash in tools.cpp at line 90, which is the while statement below:

u32 righttrim(u8* buf, u32 len){
    if(len==0){
        return len;
    }
    u8* pos = buf+len-1;
    while(pos>=buf && *pos==' '){
        pos--;
    }
    return (unsigned int)((pos-buf)+1);
}

I see this crash with 2 occurrences from a single installation of my iOS app, via Parse crashreporting. Here's the full stack that I have...

Thread 10 (crashed)
0
Photobooth
trimspaces(unsigned char**, unsigned int*) @ 0x4, tools.cpp : 90
1
Photobooth
SSDPParser::Parse(sockaddr*, unsigned char*, unsigned int) @ 0x8, ssdpparser.cpp : 165
2
Photobooth
SSDP::IncommingMessage(sockaddr*, unsigned char*, unsigned int) @ 0x10, ssdp.cpp : 406
3
Photobooth
SSDP::ReadLoop() @ 0x8, ssdp.cpp : 370
4
Photobooth
SSDP::sReadLoop(void*) @ 0x0, ssdp.cpp : 455
5
libsystem_pthread.dylib
_pthread_body @ 0xa0
6
libsystem_pthread.dylib
_pthread_start @ 0x9c
7
libsystem_pthread.dylib
thread_start @ 0x0

Here are the other two threads from upnpx in case this helps:

Thread 6
0
libsystem_kernel.dylib
__select @ 0x8
1
Photobooth
SocketServer::ReadLoop() @ 0xc, socketserver.cpp : 287
2
Photobooth
SocketServer::sReadLoop(void*) @ 0x0, socketserver.cpp : 383
3
libsystem_pthread.dylib
_pthread_body @ 0xa0
4
libsystem_pthread.dylib
_pthread_start @ 0x9c
5
libsystem_pthread.dylib
thread_start @ 0x0

Thread 7
0
libsystem_kernel.dylib
__semwait_signal @ 0x8
1
libsystem_c.dylib
nanosleep @ 0xd4
2
libsystem_c.dylib
sleep @ 0x2c
3
Photobooth
SSDPDB::CacheControlLoop() @ 0x0, ssdpdb.cpp : 264
4
Photobooth
SSDPDB::sCacheControlLoop(void*) @ 0x0, ssdpdb.cpp : 294
5
libsystem_pthread.dylib
_pthread_body @ 0xa0
6
libsystem_pthread.dylib
_pthread_start @ 0x9c
7
libsystem_pthread.dylib
thread_start @ 0x0

Thread 8
0
libsystem_kernel.dylib
__semwait_signal @ 0x8
1
libsystem_c.dylib
nanosleep @ 0xd4
2
libsystem_c.dylib
sleep @ 0x2c
3
Photobooth
-[UPnPDB httpThread:] @ 0x0, UPnPDB.m : 320
4
Foundation
__NSThread__main__ @ 0x42c
5
libsystem_pthread.dylib
_pthread_body @ 0xa0
6
libsystem_pthread.dylib
_pthread_start @ 0x9c
7
libsystem_pthread.dylib
thread_start @ 0x0

thanks!

[fkuehne]

Thanks for the report!

Regrettably, I don't have capacity at this point to fix issues which don't occur in our own products.

[lightbow]

I'll bet it's a problem is up inSSDPParser::Parse, with a garbage fieldvalue for thisHeader. Do you have an example of header data this is failing on? Otherwise, it's probably worth reading through the Parse method, looking for mishandled boundary conditions, to determine how that pointer could end up being garbage.

[xaphod]

Sorry I don't have the header data, because I don't have the repro. This is a live app that has this crash -- someone on planet earth is hitting it... ;)

On Tue, May 26, 2015 at 4:19 PM, Lightbow notifications@github.com wrote:

I'll bet it's a problem is up inSSDPParser::Parse, with a garbage fieldvalue for thisHeader. Do you have an example of header data this is failing on? Otherwise, it's probably worth reading through the Parse method, looking for mishandled boundary conditions, to determine how that pointer could end up being garbage.

Reply to this email directly or view it on GitHub https://github.com/fkuehne/upnpx/issues/62#issuecomment-105540165.

[lightbow]

Looking at the code, we'll enter this part of the Parse function. We know linelen > 0, and it looks like the code is expecting something of the form FIELDNAME:FIELDVALUE. Some edge case is tripping things up, and maybe we can guess the edge case.

            //Read the headers
            //Find the first colon, that is the end of the field name, the rest is the field value
            colon = 0;
            while(pos[colon] != ':' && colon < linelen){
                colon++;
            }

            //Add the header to our collection
            SSDP_HTTP_HEADER *thisHeader = (SSDP_HTTP_HEADER*)malloc(sizeof(SSDP_HTTP_HEADER));
            thisHeader->fieldname = pos;
            thisHeader->fieldnamelen = colon;
            thisHeader->fieldvalue = lefttrim(pos+colon+1, linelen-colon-1);
            thisHeader->fieldvaluelen = (unsigned int)(linelen-(thisHeader->fieldvalue - pos));
            //Trim spaces
            trimspaces(&(thisHeader->fieldname), &(thisHeader->fieldnamelen));
            trimspaces(&(thisHeader->fieldvalue), &(thisHeader->fieldvaluelen));

Your stack trace indicates the crash comes from down inside the last line (trimspaces calling righttrim), so we know that somehow in this case "thisHeader->fieldvalue" is toast, and "thisHeader->fieldvaluelen" is >0 (otherwise we'd hit an early return and never get to line 90 in righttrim where the crash happens). Any ideas?

[zakariachowdhury]

I had similar issue. thisHeader->fieldvaluelen is an unsigned int. When it gets negative value, it overflows to maximum unsigned int 4294967295. This causes EXC_BAD_ACCESS crash.

To fix the issue, I added a checking in ssdpparser.cpp to make sure trimspaces() is executing only when it has positive value.

//Trim spaces
trimspaces(&(thisHeader->fieldname), &(thisHeader->fieldnamelen));
if(linelen > (thisHeader->fieldvalue - pos)) {
    trimspaces(&(thisHeader->fieldvalue), &(thisHeader->fieldvaluelen));
}

[bluegaspode]

The change is not fixing the root cause of the problem, but just circumvents it.

The problem is not the trimspaces, the problem is that 'thisHeader->fieldvaluelen' gets assigned a wrong value in case of a malformed SSDPMessage.

You can reproduce the crash and debug it easily by (temporarily) inserting the following line in SSDP::Parse

pos=(u8*) & "HTTP/1.1 200 OK\r\nNO-COLON-CRASH\r\n";
len=33;

For such a message thisHeader->fieldvaluelen should be assigned to zero (but right now gets a negative number that is then casted to unsigned int).

When thisHeader->fieldvaluelen is assigned the correct value, you don't need the extra check before trimspaces.

My suggestion for a proper fix is as follows:

        thisHeader->fieldvalue = pos+colon+1;
        if (colon<linelen) {
          thisHeader->fieldvaluelen = linelen-colon-1;
        } else {
          thisHeader->fieldvaluelen = 0;
        }

The code is much cleaner and removes the warning for the cast to unsigned int. This also removes the lefttrim() for the fieldvalue, as trimspaces is called some lines below anyways.

[fkuehne]

wow, nice. Thanks for coming back to this old report and suggesting a correct fix. Would you mind sharing a patch?

Sorry for the delay in answering. Will be quick from now on again ;)

[bluegaspode]

Hi Felix,

yeah I stumbled about this crash in my App as well (using a version of upnpx from a long time ago :) ). I created a pull request with the improved fix.

This one sets the header values correctly, so code down the line will not get into trouble as well.

Cheers Stefan

[xaphod]

... itching to pod update - right after the game :) On Sat, Jul 2, 2016 at 2:48 PM bluegaspode notifications@github.com wrote:

Hi Felix,

yeah I stumbled about this crash in my App as well (using a version of upnpx from a long time ago :) ). I created a pull request with the improved fix.

This one sets the header values correctly, so code down the line will not get into trouble as well.

Cheers Stefan

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/fkuehne/upnpx/issues/62#issuecomment-230116439, or mute the thread https://github.com/notifications/unsubscribe/ADmfLkFNGynUSSjjjAiclHsIVMXD2NzOks5qRrKPgaJpZM4Ep-V5 .