Why am I getting a segfault when assigning a malloc()'d pointer to a char*?

Question

I have a struct with two char* pointers:

typedef struct{
    char* command;
    char* option;
} Command;

I then have a function that reads from a socket, splits the data into two parts (command & option) then populates a struct to return to the caller. However, I get a segfault at one point and I can't understand why.

Here is where I get the segfault signal:

commandStruct->command = strdupa(cmd);

Here are the declarations of my variables:

Command* commandStruct = malloc(sizeof(Command));
char *cmd, *option;
int cmdLen, optLen;

The part that's confusing me is that it's just a simple assignment, it shouldn't be this difficult. I'm pretty unfamiliar with the nuances of C, so it could be something blatantly obvious that a more experienced programmer might catch. Any help is appreciated.

EDIT: I have adjusted my first malloc to make room for more than just a pointer, but I am still having trouble. Valgrind mentioned that I was writing 8 bad bytes and that "Address 0x0 is not stack'd, malloc'd or (recently) free'd". To be more transparent I'll paste a copy of the entire function below, just incase it helps.

Command* getCommand(int cfd)
{
    Command* commandStruct = NULL; 
    char cmdStr[200];
    char *cmd = NULL, *option = NULL;
    int recieved, cmdLen, optLen;

    commandStruct = malloc(sizeof(Command));

    memset(cmdStr, '\0', sizeof(cmdStr));
    memset(commandStruct, 0, sizeof(Command));



    if(commandStruct == NULL)
    {
        fatal("sir, you malloc'd a null pointer. Memory problems?.\n");
    }

    if((recieved = recv(cfd, cmdStr, MAXLINE, 0)) == -1) errExit("recv");
    verbosePrint(opts.v, "recv'd %u bytes: %s\n", recieved, cmdStr);

    if(!strncmp(CMD_DIR, cmdStr, strlen(CMD_DIR)))
    {
        cmd = CMD_DIR;
        option = NULL;
        verbosePrint(opts.v, "set cmd to: %s\n", cmd);
    }
    else if(!strncmp(CMD_CHDIR, cmdStr, strlen(CMD_CHDIR)))
    {
        cmd = CMD_CHDIR;
        option = &cmdStr[sizeof(CMD_CHDIR)];
        verbosePrint(opts.v, "set cmd to: %s\n", cmd);
        verbosePrint(opts.v, "set option to: %s\n", option);
    }
    else if(!strncmp(CMD_PWD, cmdStr, strlen(CMD_PWD)))
    {
        cmd = CMD_PWD;
        option = NULL;
        verbosePrint(opts.v, "set cmd to: %s\n", cmd);
    }
    else if(!strncmp(CMD_PUT, cmdStr, strlen(CMD_PUT)))
    {
        cmd = CMD_PUT;
        option = &cmdStr[sizeof(CMD_PUT)];
        verbosePrint(opts.v, "set cmd to: %s\n", cmd);
        verbosePrint(opts.v, "set option to: %s\n", option);
    }
    else if(!strncmp(CMD_GET, cmdStr, strlen(CMD_GET)))
    {
        cmd = CMD_GET;
        option = &cmdStr[sizeof(CMD_GET)];
        verbosePrint(opts.v, "set cmd to: %s\n", cmd);
        verbosePrint(opts.v, "set option to: %s\n", option);
    }

    commandStruct->command = strdupa(cmd);

    if(option != NULL)
    {
        commandStruct->option = strdupa(option);        
    }

    return commandStruct;
}

You need to allocate another byte for the null terminator for the string. — templatetypedef, Dec 06 '14 at 22:42
Compile with `-Wall`. Don't cast `malloc`, it hides errors: http://stackoverflow.com/a/605858/19410 — Alex Reynolds, Dec 06 '14 at 22:43
I didn't know that about malloc, thanks for the tip. And I do compile with -Wall, I've not recieved any errors. — shermanzach, Dec 06 '14 at 22:47
To get help with questions such as "why am I getting valgrind errors" please post a [MCVE](http://stackoverflow.com/help/mcve) — M.M, Dec 07 '14 at 02:22
`strdupa` should be `strdup`. The former means to use stack space, so the pointer will be invalid once your function returns. Also check `cmd != NULL` before trying `strdup` on it.; and there could be other problems with all the `strncmp` calls. — M.M, Dec 07 '14 at 02:24
the check for the returned value from malloc HAS to be immediately after the call to malloc, not after the code has made modifications to the allocated memory. Otherwise the modifications could be at offsets from address 0 — user3629249, Dec 07 '14 at 03:48
regarding this line: if((recieved = recv(cfd, cmdStr, MAXLINE, 0)) == -1) errExit("recv"); the cmdStr was set at 200 bytes, not at MAXLINE bytes. suggest updating the declaration of cmdStr to use MAXLINE — user3629249, Dec 07 '14 at 03:51
how are the items like: CMD_PUT defined? are they null terminated? — user3629249, Dec 07 '14 at 04:01
@user3629249 this ultimately was the cause of my grief. The overstepping was writing over all of my local stack, which was causing problems with malloc that were very difficult to debug. -- I did end up figuring it out (6 hours of head banging later), but if you give this as an answer, I would be happy to mark it as correct. — shermanzach, Dec 07 '14 at 04:03

templatetypedef · Answer 1 · 2014-12-07T02:22:49.297

4

Your code

Command* commandStruct = (Command*) malloc(sizeof(commandStruct));

Is under allocating space. You're allocating enough space to store a pointer to a Command rather than a Command itself.

Try replacing this with

Command* commandStruct = malloc(sizeof(*commandStruct));

That should give you enough space. You're also under allocating space for the string you're storing. Consider using strdup instead.

Hope this helps!

edited Dec 07 '14 at 02:22

answered Dec 06 '14 at 22:46

templatetypedef

362,284
104
897
1,065

1

Shouldn't it be `Command* commandStruct = (Command*) malloc(sizeof(Command));` so that it actually allocates the space needed to store all the stuff in the struct? You can't just allocate the space for a pointer address, that won't do it. If Command was a class, you'd call `Command* commandStruct = new Command();` – MasterHD Dec 06 '14 at 23:08
1

@MasterHD I went with your suggestion -- It looks like either one works but your way seems more easily read – shermanzach Dec 06 '14 at 23:52
@templatetypedef I have tried both your suggestions but I am still having issues. Are there any other suggestions you might have? Thanks for your help so far! – shermanzach Dec 06 '14 at 23:57
@MasterHD That works too. I personally like the *commandStruct trick since it works even if the type of the variable changes, but either is fine. Also, new Command works in C++, but this is a pure C question. :-) As a result, there's no new keyword and no constructor to run. – templatetypedef Dec 07 '14 at 02:12
Downvoter - can you please let me know what I can do to improve this answer? I'm not sure what's wrong with it. – templatetypedef Dec 07 '14 at 02:16
@zsherman I unfortunately don't have the time to read over your code in more depth. Try stepping through it with a debugger - if Valgrind is giving you the error that it's reporting, you probably have a null pointer dereference that should be easy to find with gdb. Good luck! – templatetypedef Dec 07 '14 at 02:17
@MattMcNabb Answer updated. I'm not sure if you were the downvoter, but if so, where there any other updates I should make to this answer that would lead you to remove the downvote? – templatetypedef Dec 07 '14 at 02:23

Why am I getting a segfault when assigning a malloc()'d pointer to a char*?

1 Answers1