Concatenation of string and int results in segmentation fault in C

Question

I am not sure what I'm doing wrong. I'm trying to concatenate hostname with pid to create id.

char *generate_id(void) {
    int ret;
    char id[1048];
    char hostname[1024];
    pid_t pid = getpid();
    //hostname[1023] = '\0';

    if ((ret = gethostname(hostname,1024) < 0)) {
        perror("gethostname");
        exit(EXIT_FAILURE);
    }
    sprintf(id, "%s%d", pid);
    printf("hostname is %s\n", hostname);
    printf("The process id is %d\n", pid);
    printf("The unique id is %s", id);

    return id;
}

EDIT:

Updated code after reading some answers:

char *generate_id(void) {
    int ret;
    char hostname[1024];
    pid_t pid = getpid();
    //hostname[1023] = '\0';

    if ((ret = gethostname(hostname,1024) < 0)) {
        perror("gethostname");
        exit(EXIT_FAILURE);
    }

    int size = snprintf(NULL, 0, "%s%d", hostname, pid);
    char * id = malloc(size + 1);

    printf("hostname is %s\n", hostname);
    printf("The process id is %d\n", pid);
    printf("The unique id is %s\n", id);

    return id;
}

EDIT:

Working code:

char *generate_id(void) {
    int ret;
    char hostname[1024];
    pid_t pid = getpid();
    //hostname[1023] = '\0';

    if ((ret = gethostname(hostname,1024) < 0)) {
        perror("gethostname");
        exit(EXIT_FAILURE);
    }

    int size = snprintf(NULL, 0, "%s%d", hostname, pid);
    char * id = malloc(size + 1);
    sprintf(id, "%s%d", hostname, pid);
    printf("hostname is %s\n", hostname);
    printf("The process id is %d\n", pid);
    printf("The unique id is %s\n", id);

    return id;
}

sprintf `%s%d`, and then provide only `pid`? you have two formatting chars, so need to provide two values — Marc B, Apr 26 '16 at 17:43
Why do you have `%s%d` in sprintf shouldn't that just be `%d` — TheQAGuy, Apr 26 '16 at 17:44
You should cast `pid_t pid` when supplying it as `int` argument. `sprintf(id, "%d", (int)pid);` — Weather Vane, Apr 26 '16 at 17:46

score 3 · Accepted Answer · edited Jun 20 '20 at 09:12

Issue with your format string:

sprintf(id, "%s%d", pid);

Your format string has two formatters (%s for a string and %d for an int), yet you only pass an pid_t. You likely mean:

sprintf(id, "%s%d", hostname, pid);

or

sprintf(id, "%d", pid);

In your code, the %s interprets the pid as a pointer. Trying to dereference that to format the string causes the segmentation fault as it's an invalid pointer value.

Issue with your memory management:

But then there's also undefined behavior in your code: you declare id to be a stack-allocated array but you're returning that array (which decays into a pointer here). This also is wrong and may lead to a crash later on.

You need to change id to a heap-allocated array like this:

char * id = malloc(1024);

The caller of your generate_id function then needs to free the memory when it's done.

It's probably a good idea to only allocate the space you need. You can use snprintf for that like this:

// Determine how much space the string needs.
int size = snprintf(NULL, 0, "%d", pid);
// Allocate the required space plus NULL termination.
char * id = malloc(size + 1);
// Actually print the string.
sprintf(id, "%d", pid);

Thanks for the help, I updated the code in the question, when I try to print the unique id, its blank though. — Daniel Kobe, Apr 26 '16 at 18:05
That's because you only _allocate_ the space, but you're missing the actual call to `sprintf`. First, `snprintf` is called (notice the extra `n` in the name) in a special way (`NULL` and `0` as the first two arguments). This way you measure the size. Then you need to allocate the space with `malloc`, and _then_ you need to actually print into that space with `sprintf`. — DarkDust, Apr 26 '16 at 18:07

score 1 · Answer 2 · answered Apr 26 '16 at 17:45

1

Not sure where you are segfaulting but you have a few issues.

snprintf() is much safer and won't overrun the id[] buffer. sprintf could overrun the buffer

sprintf(id, "%s%d", pid) is bad as mentioned above.

return id is bad as it returns the pointer to a value on the stack. As soon as you return, the stack is no longer yours.

answered Apr 26 '16 at 17:45

Russ

31
1

You must either allocate the memory (malloc) and have the caller free it, or use a static buffer that the caller knows needs to be copied or utilized before calling that routine again, or similar methods. – Russ Apr 26 '16 at 17:58

score 0 · Answer 3 · answered Apr 26 '16 at 17:43

0

sprintf(id, "%s%d", pid);

You have two selectors %s and %d, but only one parameter (pid). You need to put in a string and an integer instead of just the integer.

answered Apr 26 '16 at 17:43

vacuumhead

479
1
6
16

Concatenation of string and int results in segmentation fault in C

3 Answers3

Issue with your format string:

Issue with your memory management:

Linked