1

Is this a proper thing to do in C ?

char *func1() {
    char *str[3] = { "so", "is", "amazing" }; 

    return str[1]; 
}

The char *func1() returns an pointer, pointing to a location in memory where the string is stored. But when the func1() returns won't the str array go out of scope and am I not returning a pointer to an object that does not exist? A dangling pointer (is this the right name?)

So I have two solutions make str global so that it never goes out of scope and the pointer always pointer to a valid memory address, but that seems dirty.

The other solutions

char *func2() {
    char *str[3] = { "so", "is", "amazing" };
    int str1Len = strlen(str[1]); 
    char *ret = (char *)malloc(str1Len) ; // create a block of mem to hold the str[1]. 
    strncpy(ret, str[1], str1Len);
    return ret; 
}

Is this correct ? What is the proper way to do this func1() or func2()? Or is there a better way?

I have been using func1() for some time and it has not created any problems for me, does this mean that func1() is doing the right thing? and func2() is unnecessary?

chqrlie
  • 131,814
  • 10
  • 121
  • 189
nnrales
  • 1,481
  • 1
  • 17
  • 26
  • 1
    `char * str[3] = { "so" , "is" ,"amazing" } ;` is declared **within** the function and ceases to exists when the function returns. (it is local to the function, and that memory is released when the function returns -- so the memory pointed to by `str[1]` no longer exists) You must either pass `str` to the function as a parameter, or allocate storage for `str` with `malloc` *within* the function and return a pointer to the new block of memory. – David C. Rankin Feb 07 '16 at 22:30
  • @DavidC.Rankin Yeah this is what I though too , but C CPP and there rules are simply too crazy. See this http://stackoverflow.com/questions/349025/is-a-string-literal-in-c-created-in-static-memory/349031#349031 – nnrales Feb 07 '16 at 22:45
  • @DavidC.Rankin: the `str` array itself has automatic storage, but its elements being pointers to string literals, that have static storage. It's OK to return one of them, pointers are returned by value. – chqrlie Feb 07 '16 at 22:46
  • @nnrales: I'm afraid DavidC.Rankin is mistaken. Your example is simple and if your actual code uses arrays of pointers to string literals, you can return elements from them. – chqrlie Feb 07 '16 at 22:48
  • 2
    If `str` does not change, make it `static const char * const str[] = ...`. That qualifies both, the array and the `char []` it points to `const`. The `static` makes it permanent and the rest makes it startup-time initialised. Your current version will setup the array for every call. – too honest for this site Feb 07 '16 at 22:49
  • @Olaf I keep hearing that the compiler is smart. Is it not smart enough to do it own its own ? This seems an obvious thing for the compiler to optimize ? I am a student and I am asking to get a general picture of things. – nnrales Feb 07 '16 at 23:11
  • No, it is not. Because he does not have to whole picture. Even iff, it cannot track all paths. Not necessary to tell us if you solved that _halting problem_; we will get the information you won the turing award. Please see my answer, I added some details. – too honest for this site Feb 07 '16 at 23:21

3 Answers3

4

In your first function, there is no problem returning str[1] as it is not pointing to a local variable, it is a pointer to a string literal. Note that string literals should be declared const:

const char *func1(void) {
    const char *str[3] = { "so", "is", "amazing" };
    return str[1]; 
}

Your second function returns a pointer to allocated space. This pointer will need to be freed at some point. The allocation is incorrect, you should allocate 1 extra byte for the final '\0'. You can use strcpy to copy the string to the allocated space, or simply use strdup() that does both:

char *func2(void) {
    const char *str[3] = { "so", "is", "amazing" };
    int size = strlen(str[1]) + 1; 
    char *ret = malloc(size);
    return strcpy(ret, str[1]);
}

Or simply:

char *func2(void) {
    const char *str[3] = { "so", "is", "amazing" };
    return strdup(str[1]);
}

NEVER use strncpy, it does not do what you think it does. It is very error prone for both the programmer and whoever will read your code.

chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • Sorry for the stupid question but isn't the string literal a local variable ? – nnrales Feb 07 '16 at 22:34
  • @nnrales It has [static storage duration](http://stackoverflow.com/a/349031/1983495), it doesn't matter if it's a local variable. And it's a smart question if anything. – Iharob Al Asimi Feb 07 '16 at 22:37
  • @iharob thank you the static storage duration page . I did not know anything about it. – nnrales Feb 07 '16 at 22:46
1
  1. Is this a proper thing to do in C ?

    No, it's not! You need to create a copy of the local variable in general, although in this case the pointers point to string literals so it would work.

  2. You are allocating and copying the string wrong, you should do this instead

    size_t size =  strlen(str[1]) + 1; 
    char *result = malloc(size) ; // create a block of mem to hold the str[1]. 
    if (result != NULL)
        memcpy(ret, str[1], size);
    
Iharob Al Asimi
  • 52,653
  • 6
  • 59
  • 97
  • 2
    `str[1]` is not pointing to a local variable, it is a pointer to a string literal. There is not problem returning it, except maybe for constness. – chqrlie Feb 07 '16 at 22:29
  • Why do you prefer memcpy to strncpy ? So it is alright if I write a program which uses func1() ? Why is it so ? I though when a functions returns every object created on the stack is technically de-allocated ? – nnrales Feb 07 '16 at 22:31
  • @chqrlie I said that, I said in general it's not a good idea but since they are string literals. – Iharob Al Asimi Feb 07 '16 at 22:31
  • 1
    `strncpy()` is not for this, it will try to detect the end of the string before `length` is reached (*it copies at most `length` bytes*), `memcpy()` will copy the exact number of bytes you tell it to copy. Since you have used `strlen()` then the `'\0'` better be in the source string or undefined behavior will occur. So you are sure that it is there then copy `length` bytes that's all. But also, on ocasions it will not copy the `'\0'` terminator so as a general rule **do not use `strncpy()`**. – Iharob Al Asimi Feb 07 '16 at 22:32
  • 2
    `strncpy` is not advisable anywhere. – chqrlie Feb 07 '16 at 22:32
  • @chqrlie I wish I knew this earlier , my program was crashing when I was using strncpy so , I implemented a local strncpy which does the thing that the strncpy is supposed to do and now it works. I can't change it to memcpy as it was an assingment. – nnrales Feb 07 '16 at 22:39
  • If you can't use `memcpy()` then use `strdup()` or `strcpy()`. – Iharob Al Asimi Feb 07 '16 at 22:40
  • @nnrales: How do you know what `strncpy` was *supposed* to do? The Standard semantics are what they are, contorted and error prone, do not use the function, write your own if you want, but do not name it `strncpy`. In your example, you should just allocate one more byte and use `strcpy`. – chqrlie Feb 07 '16 at 22:55
  • @chqrlie I understand and agree. – Iharob Al Asimi Feb 07 '16 at 23:00
0

While it is acceptable to return pointers to the string literals, buit not to a local pointer, it might be the wrong approach.

If str does not change, make it static const char * const str[] = .... That qualifies both, the array and the char [] (which is each string literal) it points to const. The static makes it permanent, but with still local scope. It is setup once before your program code starts. In contrast, a non-static version will setup the array for every call.

Note that you have to return a const char * to maintain const-correctness. I strongly recommend that; if you cannot, omit the first const only.

Note: Do not cast the result of malloc & friends (or void * in general) in C.

too honest for this site
  • 12,050
  • 4
  • 30
  • 52
  • Thank you , I implemented what you said but to return it as a char * I had to drop the const. My Prof, tells me to always cast the result of malloc and void * into the type that I know it is . Why do you say other wise ? – nnrales Feb 07 '16 at 23:18
  • 1
    @nnrales: Sorry, but your prof is blatanic wrong! He might want to read the C [standard](http://port70.net/~nsz/c/c11/n1570.html). Also there is a thread here already about that subject, see the info-page for this and more. Maybe he should learn C is not C++ (in C++ you have to) and shall not use a C++ compiler to compile C code. Casts inhibit type-checking,as they tell your compiler to "shut up" and not to warn you. Only use a cast if you 1) have to 2) really understand **all** implications and 3) accept them completely. – too honest for this site Feb 07 '16 at 23:27
  • 1
    @nnrales: About const-correctness: I strongly recommend to keep that and - as I wrote - correct the other code. `const`-correctness is an important measure to make your code safer and help your compiler tell you if something is fishy with your code. – too honest for this site Feb 07 '16 at 23:27
  • Thanks for your tip. My array is quite large and reallocating it each time the function is called , will be expensive. – nnrales Feb 07 '16 at 23:30
  • 1
    For a large array, it might be better to make it file-local. Just move it as-is outside the function (leave it `static`). That for readability and to keep your functions source code footprint small. – too honest for this site Feb 07 '16 at 23:31