CODEWITHSUNDEEP
copenssl
user20226941
user20226941

Reputation:

Segmentation fault strcat

I got a problem when reading SSL response that causes a segmentation fault. I read the response into a buffer, then append it to a malloced string and memory reset it to 0 till the response is fully read, but when I try this in a multi threaded program, after some operations it gives me segmentation fault. When I remove strcat it doesn't give me segmentation fault even if I run it for hours.

Example:

char* response = malloc(10000);
char buffer[10000] = { 0 };

while(SSL_read(ssl,buf,sizeof(buffer)) > 0){
    strcat(response,buffer);
    memset(buffer,0,sizeof(buffer));
}

Errors

free(): invalid next size (normal)
malloc_consolidate(): invalid chunk

I made sure of freeing both of SSL and CTX and close socket and free the malloced string.

Upvotes: 0

Views: 191

Answers (2)

Marco Bonelli
Marco Bonelli

Reputation: 69326

There are a few problems with your code:

  1. You are not dealing with C strings, you are dealing with arbitrary byte sequences. SSL_read() reads bytes, not C strings, and you cannot treat them as strings. What you read cannot be assumed to be NUL-terminated (\0), so you should not use strcat, strlen or other similar functions that operate on strings. Zeroing out the entire buffers just to make sure there is a terminator makes little to no sense, as the terminator could very well be found in the middle of the data.

  2. You are reading data continuously in a loop into a fixed size buffer. Your code will overflow the destination buffer (response) very easily.

  3. Not an error, but there isn't really any need for an intermediate buffer to begin with. You are needlessly copying stuff around two times (one with SSL_read() and one with strcat) when you can read directly into response instead. On top of that, the memset() to clear the contents of buffer also adds a third scan of the data, slowing things down even more.

  4. Again, not an error, but SSL_read() returns int and uses that to return the read size. You are not really using it, but you should, as you need to keep track of how much space is left on the buffer. You would be however much better off using size_t to avoid unwanted problems with signed math and possible overflows. You can use SSL_read_ex() for this purpose.

Here's a snippet of code that does what you want in a more robust way:

#define CHUNK_SIZE 10000

unsigned char *response = NULL;
size_t size = 0;
size_t space_left = 0;
size_t total_read = 0;
size_t n;

while (1) {
    // Allocate more memory if needed.
    if (space_left < CHUNK_SIZE) {
        unsigned char *tmp = realloc(response, size + CHUNK_SIZE);
        if (!tmp) {
            // Handle realloc error
            break;
        }

        response = tmp;
        size += CHUNK_SIZE;
        space_left += CHUNK_SIZE;
    }

    if (SSL_read_ex(ssl, response + total_read, space_left, &n)) {
        total_read += n;
        space_left -= n;
    } else {
        // Handle error
        break;
    }
}

Upvotes: 2

Barmar
Barmar

Reputation: 780974

You never initialized response(). The arguments to strcat() have to be null-terminated strings.

You should also subtract 1 from the size of the buffer when calling SSL_read(), to ensure there will always be room for its null terminator.

char* response = malloc(10000);
response[0] = '\0';
char buffer[10000] = { 0 };

while(SSL_read(ssl,buf,sizeof(buffer)-1) > 0){
    strcat(response,buffer);
    memset(buffer,0,sizeof(buffer));
}

Upvotes: 0

Related Questions