-1

I have a simple function to handle the parsing and hashing of the Sec-WebSocket-Key value in a C websocket program. I had the whole program working but found out I had a bunch of char* without a static location to point to. As you may have guessed this caused some memory issues and needed to be fixed. To fix the issue I made char's of size 100 and pointed the char* at them. Now the values I am getting back from the function are incorrect. Can someone tell me what I am doing wrong. From my understanding this should work. Fyi I am self taught and still have huge gaps in my C understanding.

char* sock_handle_hash(struct sock_data *sockdat, int dataCount) { 
    char EncodeHashbuff[100];
    char key1buff[100];
    char key2buff[100];
    char key3buff[100];
    char *EncodeHash = EncodeHashbuff;
    char *key1 = key1buff;
    char *key2 = key2buff;
    char *key3 = key3buff;


    unsigned char hash[SHA_DIGEST_LENGTH]; // this sets the length to the predefigned length in the SHA standard
    char *testKey = "Sec-WebSocket-Key"; // this is the key for the key value pair of the hash
    char *additionalHashData = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11"; // the magic string used in websockets
    sockdat->buffer[dataCount] = '\0'; // null terminate the buffer
    key1 = strtok(sockdat->buffer, "\n"); // brake up the data by new lines
    key1 = strtok(NULL, "\n"); // skip the first line
    while (key2 != NULL) { //find the key to hash
        key2 = strtok(NULL, ":"); //brake data into the key value pairs
        key3 = strtok(NULL, "\n"); // go to next line
        if(strcmp(key2, testKey) == 0) { // if the correct key
            if( key3[(strlen(key3)-1)] =='\r'){
                key3[(strlen(key3)-1)]='\0';
            }
            key3++;
            char key4[200];
            strcpy(key4, key3); // copy the string to the final key
            strcat(key4, additionalHashData); // concat the magic websocket hash string
            SHA1(key4, strlen(key4), hash); //Hash SHA1 to the correct reply value
            EncodeHash = apssock_base64(hash, sizeof hash); // base 64 encode the value
            break; //Stop looping 
        }                   
    }   

    return EncodeHash; //success retrun the hashed value for the handshake
}
mando222
  • 543
  • 3
  • 6
  • 16
  • What is a "char of size 100"? Do you mean a `char [100]`, i.-e. an array of `char`? And see [ask]. – too honest for this site Apr 04 '16 at 18:35
  • `return EncodeHash;` may as well be `return EncodeHashbuff;`, which returns the address of an expiring automatic variable in the function's scope. Thus, undefined behavior when the caller utilizes that returned address. [**Read this answer**](http://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope/6445794#6445794) carefully. – WhozCraig Apr 04 '16 at 18:47
  • Fyi, unless `EncodeHash = apssock_base64(hash, sizeof hash);` dynamically allocates the resulting base64 encoding, which we don't know from the posted code, that code path will likewise suffer a similar fate. If it does dyna-alloc, it *may* work (at least it won't invoke the UB mentioned above). – WhozCraig Apr 04 '16 at 19:00
  • Thank you WhozCraig. That was a very informative link. Still working the issue though. – mando222 Apr 04 '16 at 19:46

1 Answers1

0

As WhozCraig stated I was referencing something that had no guarantee of existing anymore. To fix this I added a static to the line:

static char EncodeHashbuff[100];
mando222
  • 543
  • 3
  • 6
  • 16
  • FYI: This solution isn't thread-safe, since the memory will be shared among threads. if your server is multi-threaded, this will fail. Consider changing the function's prototype to accept a buffer to be written upon (this way, the `char` array can be created normally on the stack by the calling function). i.e. `char* sock_handle_hash(struct sock_data *sockdat, int dataCount, char * EncodeHash) ` – Myst Apr 11 '16 at 12:43
  • Interesting. Can you provide a good link to more on that subject? Thanks. – mando222 Apr 11 '16 at 20:26
  • There's some explanations about static variables and thread safety [here](http://stackoverflow.com/questions/16347928/function-using-a-local-static-variable-thread-safe-reentrant) and [here](http://stackoverflow.com/questions/1270927/are-function-static-variables-thread-safe-in-gcc). As for writing the data to an existing char buffer, you can see some answers [here](http://stackoverflow.com/a/1496328/4025095) [here](http://stackoverflow.com/a/25799033/4025095) or [here](http://stackoverflow.com/a/12575344/4025095). – Myst Apr 12 '16 at 01:36