[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freeamp-dev
Subject:    Evil in Http.cpp?
From:       Chris Kuklewicz <chrisk () MIT ! EDU>
Date:       2000-09-21 7:30:52
[Download RAW message or body]

Oh this is just evil.

Run freeamp, Open MyMusic, Click on triangle to expand streams.
Wait for 1-3 sec.  Segfault.

setting print statements and compiling with -O0 and attaching to a
running freeamp with gdb helped locate the segfault.

even looking at m_buffer (which was not null) caused a crash, It was
a bad memory address.

how can this be? It is set only by 
    m_buffer = new unsigned char[m_bufferSize];

When StreamTimer calls DownloadToString in browsertree, the
Download call in DownloadToString returns with m_bytesinBuffer zero.

how can m_buffer be messed up? perhaps another command is putting
bytes into that memory space.  So I start looking in Http::Download
and I find:

            const char* kHTTPQuery = "GET %s HTTP/1.1\r\n"
                                     "Host: %s\r\n"
                                     "Accept: */*\r\n" 
                                     "User-Agent: FreeAmp/%s\r\n";

            // the magic 256 is enough for a time field that
            // we got from the server
            char* query = new char[ strlen(kHTTPQuery) + 
                                    strlen(file) +
                                    strlen(localname) +
                                    strlen(FREEAMP_VERSION)+
                                    2];
        
            sprintf(query, kHTTPQuery, file, localname, FREEAMP_VERSION);
            strcat(query, "\r\n");

It would be better to use snprintf.  Also the +2 in the size of
the query array size has nothing to do with why query does not
overflow.  It does not overflow becase the three "%s" substrings
are deleted, making room.  If the +2 were needed to fit the "\r\n"
then the last '\0' byte written by strcat would overflow.

So this code works, but it looks like the author's thought process
was wrong, and the +2 is very very misleading.

Why not just add another \r\n to the kHTTPQuery format string and
remove the strcat?  Oh..and use snprint.  Or at least strncat.  And
for my sake, put paranoid check before calling memcpy. And check the
return values!  My Http.cpp is a bit hacked up right now so I can't
easily submit a patch for that right now.

Right before Download calls WriteToBuffer I see:
m_bufferSize=0, m_bytesInBuffer=0, m_buffer=(null)

Right after Download calls WriteToBuffer I see:
m_bufferSize=1804, m_bytesInBuffer=0, m_buffer=
Segmentation Fault

I'm going to make clean;make to reset the binaries....
_______________________________________________
FreeAmp-dev@freeamp.org
http://www.freeamp.org/mailman/listinfo/freeamp-dev

[prev in list] [next in list] [prev in thread] [next in thread]