CODEWITHSUNDEEP
cstringmemory-management
Doldrim
Doldrim

Reputation: 495

Bug trying to add two strings with snprintf

I'm trying to add two strings with snprintf but apparently i dont know what i'm doing.

Here is the code block:

char * filename = NULL;

(void)snprintf (filename, sizeof(filename), "%s/%s",
        PATH, FILE);

I also tried:

char * filename = NULL;

(void)snprintf (filename, sizeof(PATH)+sizeof(FILE)+1, "%s/%s",
        PATH, FILE);

PATH and FILE are header defined strings. Occassionally, this code works, occassionally it does not. I'm sure it's some kind of memory issue, what have I done wrong?

EDIT: My issue was for some reason thinking that snprintf allocated memory for you. I'm accepting the answer that cleared that up, since it was my real issue, but I've decided to go with the compile time string concatenation since that is a really nice trick.

Upvotes: 2

Views: 1401

Answers (7)

Brian
Brian

Reputation: 25834

Add this code:

if (!(filename = malloc((LEN + 1) * sizeof(char))))
        return 1;

SET LEN to some value >= PATH and FILE length. Don't forget to deallocate when done.

Upvotes: 0

Rob Parker
Rob Parker

Reputation: 4196

I would've just added this as a comment to GMan's answer, but until they merged my account I didn't have the rep to do so. (And now GMan's answer has apparently been deleted, so this makes less sense.)

I think GMan meant

unsigned bufferSize = strlen(PATH) + strlen(FILE) + 2; // "/" and null-terminator

to get the lengths of those constants, not the size of the pointers as slipbull points out. (Edited: Or, if sizeof(STRINGLITERAL) gives the size of the data, including the ending null, then GMan's code would still work, for string constants.)

Although, I think if you know the exact lengths of PATH and FILE (as bufferSize) you shouldn't need snprintf(), just sprintf() would be safe. But there's no harm in using snprintf() since you need the bufferSize for the allocation, anyway.

Edited: Or, Andrew Y may have an even better approach (for compile-time string constants), if that can apply for you.

Upvotes: 0

ephemient
ephemient

Reputation: 204876

On some systems, you can use asprintf:

char *filename = NULL;
asprintf(&filename, "%s/%s", PATH, FILE);

As far as I know, asprintf is a Glibc invention, but has proven so useful that several other libcs have also implemented it.

Upvotes: 0

Christoph
Christoph

Reputation: 169673

You know the length of the string at compile-time, so there's no need for dynamic allocation. 

static char filename[sizeof(PATH) + sizeof(FILE)];
snprintf(filename, sizeof(filename), "%s/%s", PATH, FILE);

But as the strings you want to join are most likely given as literals, there's no need for snprintf() at all:

static const char filename[] = PATH "/" FILE;

Also, because there's some confusion about the string's length:

strlen(PATH) + strlen("/") + strlen(FILE) + 1
= (sizeof(PATH)-1) + 1 + (sizeof(FILE)-1) + 1
= sizeof(PATH) + sizeof(FILE)

Upvotes: 2

dfa
dfa

Reputation: 116372

there are several problems in your code:

  • you are declaring a pointer, char * filename without allocating memory for it.

    1. using sizeof() for pointers (to char*) is really dangerous:

char hello[] = "world";  /* yield 6 */
char *hello2 = hello;    /* yield 4 or 8 */

so you must first allocate enough memory for holding the filename:

size_t filesize = strlen(PATH) + strlen(FILE) + 2; /* \0 + / */
char *filename = malloc(filesize);

if (filename == NULL) {
     /* error handling */
}

/* now you can safely write to filename: */
snprintf(filename, filesize, "%s/%s", FILE, PATH);

alternatively you can just allocate a big array of char:

char filename[1024]; // or #define a MAXPATH lenght
snprintf(filename, filesize, "%s/%s", FILE, PATH);
/* hoping that filename lenght is less than 1024 or MAXPATH... */

Upvotes: 0

Andrew Y
Andrew Y

Reputation: 5107

If FILE and PATH are defined in the headers as string literals, then you could concatenate at compile time:

#include <stdio.h>
/* elsewhere in your headers */
#define FILE "foo.ext"
#define PATH "/dir/subdir"

/* After including those headers */
#define FULLPATH (PATH "/" FILE)

int main(int argc, char *argv[]) {
  printf("%s\n", FULLPATH);
}

Or just do it directly when declaring a variable and reference it elsewhere in your code:

#include <stdio.h>

#define FILE "foo.ext"
#define PATH "/dir/subdir"

char fullpath[] = PATH "/" FILE;

int main(int argc, char *argv[]) {
  printf("%s occupies %d bytes\n", fullpath, sizeof(fullpath));
}

Upvotes: 5

Sinan Taifour
Sinan Taifour

Reputation: 10805

You should allocate memory first.

char * filename = NULL;
filename = malloc(sizeof(PATH) + sizeof(FILE) + 1);
snprintf (filename, sizeof(PATH) + sizeof(FILE) + 1, "%s/%s", PATH, FILE);

Upvotes: 4

Related Questions