CODEWITHSUNDEEP
cstringpointers
mstagg
mstagg

Reputation: 497

Proper way to return a string in C

I have the following code:

char* get_address_string(PACKAGE* pkg){
    char *c;
    sprintf(c, "%02x:%02x:%02x:%02x:%02x:%02x", pkg->address[0], pkg->address[1], 
        pkg->address[2], pkg->address[3], pkg->address[4], pkg->address[5]);
    return c;
}

The code works fine. However, I know this is not the proper way to return a string in C. I am receiving the warning "c is used uninitialized in this function".

What is the proper way to write this function in C?

Upvotes: 4

Views: 331

Answers (4)

chux
chux

Reputation: 154582

"Proper way to return a string in C" is not truly possible. In C, a string is a character array (up to and including the null character) and arrays, by themselves, cannot be returned from a function.

A function can return pointers. So the usual method of "return a string" it to:

  1. Return a pointer. char *foo1(...) like char *strdup()

  2. Pass in a pointer to a character array and modify its contents. void foo2(char *,...) like int sprintf(char *dest, const char *format, ...)

  3. Combine 1 & 2 char *foo3(char *, ...) like char *strcpy(char *dest, char *src)

  4. Pass the address of a pointer and update that. foo4(char **ptr) like ssize_t getline(char **lineptr, size_t *n, FILE *stream)

The key is that the memory associated with the pointer must be valid after the function is complete. Returning a pointer to a function's non-static memory is undefined behavior. Successful methods include having the calling code pass in the pointer, or the function providing it via memory allocation of pointer to some persistent value like a global variable or string constant.

What is the proper way to write this function in C?

Current design practice encourages functions like #2 & #3 above to also supply a size_t size so the function knowns the limitations of the memory available.

    char *foo2(char *s, size_t size, const pkg_T *pkg) {
      int result = snprintf(s, size, "%02x:%02x:%02x:%02x:%02x:%02x", 
        pkg->address[0], pkg->address[1], pkg->address[2], 
        pkg->address[3], pkg->address[4], pkg->address[5]);
      // encoding error or not enough room
      if (result < 0 || result >= size) return NULL;
      return s;
    }

Another method would allocate memory (I favor the above though). This obliges the calling code to free() the memory.

    #define UINT_MAX_WIDTH (sizeof(unsigned)*CHAR_BIT/3 + 3)

    char *foo2alloc(char *s, size_t size, const pkg_T *pkg) {
      char buf[(UINT_MAX_WIDTH+3)*6 + 1];
      int result = snprintf(buf, sizeof buf, "%02x:%02x:%02x:%02x:%02x:%02x", 
        pkg->address[0], pkg->address[1], pkg->address[2], 
        pkg->address[3], pkg->address[4], pkg->address[5]);
      // encoding error or not enough room
      if (result < 0 || result >= size) return NULL;
      return strdup(buf);
    }

Upvotes: 9

dbush
dbush

Reputation: 225767

Since c is uninitialized, sprintf writes to an unknown memory location, which leads to unspecified behavior. It might crash immediately, it might not crash at all, or it might crash on some completely unrelated line of code.

You need to initialize the pointer by allocating memory to it with malloc.

char* get_address_string(PACKAGE* pkg){
    char *c = malloc(20);  // enough room for output as 00:11:22:33:44:55 plus null terminator
    if (c == null) {
        perror("malloc failed");
        exit(1);
    }
    sprintf(c, "%02x:%02x:%02x:%02x:%02x:%02x", pkg->address[0], pkg->address[1], pkg->address[2], pkg->address[3], pkg->address[4], pkg->address[5]);
    return c;
}

Note that even though you know ahead of time how much memory you need, you can't set it aside at compile time via an array. This is wrong:

char* get_address_string(PACKAGE* pkg){
    char c[20];    // allocated on the stack, contents unspecified on return
    sprintf(c, "%02x:%02x:%02x:%02x:%02x:%02x", pkg->address[0], pkg->address[1], pkg->address[2], pkg->address[3], pkg->address[4], pkg->address[5]);
    return c;
}

As is this:

char* get_address_string(PACKAGE* pkg){
    char c[20];    // allocated on the stack, contents unspecified on return
    char *p = c;
    sprintf(p, "%02x:%02x:%02x:%02x:%02x:%02x", pkg->address[0], pkg->address[1], pkg->address[2], pkg->address[3], pkg->address[4], pkg->address[5]);
    return p;
}

Since c is allocated on the stack, when get_address_string returns the contents are unspecified, leading again to unspecified behavior.

Upvotes: 0

Kenny Lim
Kenny Lim

Reputation: 1214

I prefer allocating heap from the caller so that it's clear who should free it.

#include <stdio.h>
#include <malloc.h>

bool GetString(char ** retString, size_t size)
{
    // use size to do range check
    sprintf_s(*retString, size, "blah blah blah");

    return true;
}

int _tmain(int argc, _TCHAR* argv[])
{
    size_t size = 100;
    char *data = (char *)malloc(size);

    if (data)
    {
        GetString(&data, size);

        free(data);
    }
    return 0;
}

Upvotes: -3

Ely
Ely

Reputation: 11174

c is a pointer, but no memory is allocated. The return value is ok, that's how it can be done in C.

But you need to allocate memory.

Upvotes: 3

Related Questions