0

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;
}
Improve this question
16
  • Please define "not working as expected". Commented Aug 5, 2015 at 12:03
  • @Marvin, this is the output I get [��] [def] [ghi] [fromfun] Commented Aug 5, 2015 at 12:04
  • 2
    'it just has to hold an address'...... 'ret[i] =' Commented Aug 5, 2015 at 12:11
  • 1
    lol - @SouravGhosh didn't get the accept anyway, even though first to the correct answer:( Commented Aug 5, 2015 at 12:27
  • 2
    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. Commented Aug 5, 2015 at 12:35

2 Answers 2

Reset to default
6

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,

  1. Always check for the success of malloc() before using the returned pointer
  2. In C, sizeof(char) is guaranteed to be 1. Using the same as a multiplier is redundant.
Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

+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!)
@Olaf Thank you for your valuable input. I tried to clarify, please review once more. :-)
Perfectly. If I had asked I had accepted your answer. (how polite we are today:-). I just took the freedom to add two spaces.
@Olaf Fine, fine. Good to hear that. :-)
5

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;
}
Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.