returning array of string from function not working as expected

Question

I am trying to pass an array of strings to a function, make some changes to it inside this function, and pass it back to main() and print it to see the changes. It is not working as expected. Please tell me where I'm going wrong.

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

//don't forget to declare this function
char** fun(char [][20]);

int main(void)
{
    char strar[10][20] = { {"abc"}, {"def"}, {"ghi"}, {""},{""} }; //make sure 10 is added
    char** ret; //no need to allocate anything for ret, ret is just a placeholder, allocation everything done in fun
    int i = 0;

    ret = fun(strar);
    for(i=0;i<4;i++)
        printf("[%s] ",ret[i]);

    printf("\n");
    return 0;
}

//don't forget function has to return char** and not int. (Remember char**, not char*)
char** fun(char strar[][20])
{
    int i = 0;
    char** ret;
    ret = malloc(sizeof(void*)); //sizeof(void*) is enough, it just has to hold an address 

    for(i=0;i<5;i++)
    {
        ret[i] = malloc(20 * sizeof(char));
        strcpy(ret[i],strar[i]);
    }

    strcpy(ret[3],"fromfun");

    return ret;
}

@Marvin, this is the output I get [��] [def] [ghi] [fromfun] — saltandwater, Aug 05 '15 at 12:04
lol - @SouravGhosh didn't get the accept anyway, even though first to the correct answer:( — Martin James, Aug 05 '15 at 12:27
@MartinJames What to do.. :'( Today's not really my day. Did you notice the serial DV on me earlier today? (weep) — Sourav Ghosh, Aug 05 '15 at 12:28
Note: Do not use `void *` without actual need. The correct way is `ret = malloc(sizeof(*ret))`. This will infer the type from the variable, thus always have the correct size. The second best `ret = malloc(sizeof(char *))`. Note: I intentionally omitted the required multiplier. Also note: `sizeof(char)` is _defined_ to yield `1`. I.e., it will never yield anything different, so it is nonsense to use. — too honest for this site, Aug 05 '15 at 12:35
@SouravGhosh: Fine, thanks, hope you're fine, too. Ok, got it. Yes, sorry, I did not hav a closer look at it. Anyway, it might help, if as many people comment on that as possible. — too honest for this site, Aug 05 '15 at 13:31
@Olaf Yes, sure sir. Absolutely perfect, no issues. Take my comment in this way, I was trying to get a _free_ review of my answer from you by pointing you to it. Anyways, I ^-ed your comment already. :-) — Sourav Ghosh, Aug 05 '15 at 13:36
@SouravGhosh - no I didn't notice but, anyway, it was your turn to suffer:) — Martin James, Aug 05 '15 at 15:24

score 6 · Answer 1 · edited Aug 05 '15 at 14:35

The major issue, as I can see is the memory overrun.

You allocate memory to hold one element

 ret = malloc(sizeof(void*));

but, you're putting 5 elements.

for(i=0;i<5;i++)
{
    ret[i] = malloc(20 * sizeof(char));....

It is undefined behaviour. to access beyond the allocated memory.

The memory allocation to ret should look like

 ret = malloc(5 * sizeof(char *));

or

 ret = malloc(5 * sizeof*ret); //portable

To elaborate the changes made

Allocate 5 times the size of a single element, as we're going to store 5 elements.
Strictly speaking, as the ret is of type char **, we need to use char * while calculating the size to be allocated for ret, not a void *.
The change towards using sizeof *ret makes the code more robust, as in future, if the type of ret get changed to some other type, you don't need to repeat the type changes in this allocation, as the allocation would depend on the type of *ret, anyway.

Note: FWIW, the parenthesis around the argument to sizeof is required only in case of the argument being a data type, like sizeof(int). In case of using a variable name as argument, the parenthesis is optional, i.e., both sizeof(*ptr) and sizeof *ptr are both perfectly valid and legal.

That said,

Always check for the success of malloc() before using the returned pointer
In C, sizeof(char) is guaranteed to be 1. Using the same as a multiplier is redundant.

+1. I like it as you actually explain, not just provide code. Two small additions, if I may suggest: Why the change from `void *` to `char *`, and I'd put more emphasis on `*ret`. A personal note: I prefer to use function-style of `sizeof` actually; that is more consistent - but that is really just my personal preference, your code is ok as-is!) — too honest for this site, Aug 05 '15 at 14:08
@Olaf Thank you for your valuable input. I tried to clarify, please review once more. :-) — Sourav Ghosh, Aug 05 '15 at 14:27
Perfectly. If I had asked I had accepted your answer. (how polite we are today:-). I just took the freedom to add two spaces. — too honest for this site, Aug 05 '15 at 14:38

score 5 · Accepted Answer · answered Aug 05 '15 at 12:13

You need to make sure that you allocate the full array of pointers for ret array.

//don't forget function has to return char** and not int. (Remember char**, not char*)
char** fun(char strar[][20])
{
 int i = 0;
 char** ret;
 ret = malloc(sizeof(void*) * 5); //sizeof(void*) is enough, it just has to hold an address 

 for(i=0;i<5;i++)
 {
  ret[i] = malloc(20 * sizeof(char));
  strcpy(ret[i],strar[i]);
 }

 strcpy(ret[3],"fromfun");

 return ret;
}

returning array of string from function not working as expected

2 Answers2

Linked