2

I am trying to copy a char from a char type function to a var in the main function using strcpy, but the program always give me a random value as return and the program is finished. Is there some syntax error I'm missing in the code or maybe another way to do this copy?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>

char text()
{
    char palavra1[]= "tata";
    return palavra1;
}

char main()
{
    char saida[50];
    strcpy(saida,text());   // this line is supposed to copy the string from text to the var saida
    printf("%s", saida);
    return 0;
}
Pang
  • 8,605
  • 144
  • 77
  • 113
  • 1
    You must return `char*`, you cannot declare `palavral` local to `text()` and return anything (all memory for `text` is destroyed when function returns). Either pass pointer to array with sufficient storage to `text` and copy or declare `palavral` as a pointer and allocate with `malloc` and return pointer to `main` (e.g. `char *palavral = malloc (strlen ("tata") + 1); strcpy (palvral, "tata"); return palvral;`) You are responsible for freeing the memory back in `main` when you are done. – David C. Rankin Feb 05 '18 at 03:10

5 Answers5

3

In the function text(), palavra1 is stored on the stack in test()s stack frame, so when you return the string and go back to main's stack frame, the memory occupied by palavra1 is no longer kept by the compiler, and can and will be overwritten by calling other functions in main. To fix this, you have two options, either have the caller of text provide a pointer to store the string to, or have text malloc() some memory for the string and return that, relying on the calling function to free it when it is done. Personally, I'd recommend going with the first method as it means you don't have to worry about freeing the pointer when you're done, but it's really your choice.

Additionally, your function return types are both wrong. text() should return a char*, and you should hopefully be getting a warning about that from your compiler. Finally, main() specified in the ISO standard as int main(void) or int main(int argc, char** argv). It's generally bad practice to stray from either of these two signatures.

C_Elegans
  • 1,055
  • 8
  • 14
  • This is largely on track, but the stack you refer to is not part of the C specification; it is a means of implementing the things the C standard requires. In C terms, the `palavra1` object defined inside `test` ceases to exist when `test` returns, so it cannot be used to return a string. – Eric Postpischil Feb 05 '18 at 03:22
  • There is a third option for returning a string; one can return it in an object of static duration. That does not need to be explained, as it should generally be avoided, but it means it is not correct to say there are just two. The answer could say “To fix this, you have two good options…” and later mention “Technically, a third way is to return a string in an array with static storage duration, but this is generally not good design, so it is not explained here.” – Eric Postpischil Feb 05 '18 at 03:41
2

Each function can return its own type, but it cannot return a pointer to memory allocated within its function stack because the function stack is released for reuse on the function return and is no longer reachable. Any attempt to access a value whose memory is no longer reachable is Undefined Behavior.

You have two options, either (1) pass saida as a parameter to text, (e.g. char *text (char *s) { strcpy (s, "tata"); return s } (the function could be declared void at that point because the characters copied to s will be available back in the caller anyway -- and there would be no need for strcpy in main()), or (2) dynamically allocate memory to hold "tata" in text() and return a pointer to the new block of memory. Memory allocated with malloc, calloc, or realloc survives the function return as it is allocated on the heap and has program duration (or until it is freed).

(note: a 3rd option is to declare palavra1 as a static character array of sufficient size to hold "tata" -- which would also cause it to survive function return. However that practice should generally be avoided in favor of the first two options)

You could dynamically allocate in a simple fashion like:

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

char *text(void)
{
    char *p = "tata",   /* delcare pointer to string literal "tata" */
        *palavra1 = malloc (strlen (p) + 1);    /* allocate storage */

    if (palavra1 != NULL)       /* validate malloc succeeded */
        strcpy (palavra1, p);   /* copy "tata" to palavra1 */

    return palavra1;
}

int main(void)
{
    char saida[50],
        *p = text();    /* must preserve pointer to beginning of memory */

    /* validate p not NULL and length < 50 */
    if (p != NULL && strlen (p) < sizeof saida) {
        strcpy (saida, p);          /* copy to saida */
        printf ("%s\n", saida);     /* output */
        free (p);                   /* free allocated memory */
    }

    return 0;
}

Example Use/Output

$ ./bin/allocinfn
tata

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to insure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/allocinfn
==13720== Memcheck, a memory error detector
==13720== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==13720== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==13720== Command: ./bin/allocinfn
==13720==
tata
==13720==
==13720== HEAP SUMMARY:
==13720==     in use at exit: 0 bytes in 0 blocks
==13720==   total heap usage: 1 allocs, 1 frees, 5 bytes allocated
==13720==
==13720== All heap blocks were freed -- no leaks are possible
==13720==
==13720== For counts of detected and suppressed errors, rerun with: -v
==13720== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

(also note: The proper declarations for main are int main (void) and int main (int argc, char **argv) (which you will see written with the equivalent char *argv[]). note: main is a function of type int and it returns a value. See: C11 Standard §5.1.2.2.1 Program startup p1 (draft n1570). See also: See What should main() return in C and C++?)

Look things over and let me know if you have further questions.

David C. Rankin
  • 69,681
  • 6
  • 44
  • 72
1

Even if you fix the return type of text() to

char *text()
{
    char palavra1[]= "tata";
    return palavra1;
}

this is still wrong. The reason for that is that palavra1 is a local variable in text() and it is only valid and accessible until text() returns. After that the pointer to palavra1 points to an invalid location, using it to read/write values will lead to undefined behaviour.

There are two approaches for returning strings from a function.

  1. The function allocates memory with malloc and returns a pointer to the allocated memory. The function calling text() will need to free the memory:

    char *text(void)
    {
        char *ptr, palavra1[] = "tata";
    
        ptr = malloc(strlen(palavra1) + 1);
        if(ptr == NULL)
            return NULL;
    
        strcpy(ptr, palavra1);
        return ptr;
    }
    
    int main(void)
    {
        char saida[50];
        char *res = text();
    
        if(res == NULL)
        {
            fprintf(stderr, "failed to allocate memory\n");
            return 1;
        }
    
        strncpy(saida, res, sizeof saida);
        saida[sizeof(saida) - 1] = 0; // making sure to get a valid string
    
        puts(saida);
    
        free(res);
    
        return 0;
    }
    
  2. The second option is to pass a pointer to the function and let the function write the content through the pointer:

    int text(char *buffer, size_t length)
    {
        palavra1[] = "tata";
    
        if(buffer == NULL)
            return 0;
    
        strncpy(buffer, palavra1, length);
        buffer[length - 1] = 0; // making sure to get a valid string
        return 1;
    }
    
    int main(void)
    {
        char saida[50];
    
        text(saida, sizeof saida);
    
        puts(saida);
    
        return 0;
    }
    
  3. A third option (I know I said 2) would be declaring a palavra1 in text() as static char[] = "tata";. For sake of completeness I've added this third option, but I'd suggest that you don't use that, because it has other problems that you don't want to deal with, for example when your functions need to be called in a nested kind of way. So better ignore that until you really know and understand how to deal with static variables.

In both cases I've used strncpy, because with it you can limit the number of bytes written thus prevent a buffer overflow. However, strncpy might not write the '\0'-terminating byte if the destination buffer is not large enough, that's why you have to make sure that the strings is '\0'-terminated by writing the 0 at the last place of the destination buffer.

Also, the correct way of declaring the main function are:

  • int main(void);
  • int main(int argc, char **argv);
  • int main(int argc, char *argv[]);

I've never seen a char main() before, that's the first time for me. That's incorrect and you should change that.

Pablo
  • 12,254
  • 3
  • 31
  • 52
  • This is currently the best answer here, in correctness, thoroughness, and structure. I do not know why others have not voted it up. Unless there is something wrong with the code—I have not checked it thoroughly. But, if somebody noticed something, they should speak up. – Eric Postpischil Feb 05 '18 at 04:11
0

palavral has automatic storage duration - meaning when the function ends it will be destroyed (more clearly the block on which it is declared ends but here the function body is the block where it is declared - so when the function ends it's lifetime will end, as the block ends). Now you returned pointer to the local variable - precisely pointing to the memory which will not be there once the function ends and tried to access it via using it on strcpy - this is undefined behavior.

In fact standard says it :-

From §6.2.4 you can see under undefined behavior

An object is referred to outside of its lifetime

This was the problem:- but there are couple of solutions. If you want to return some pointer then either you can dynamically allocate memory and return the address of the allocated chunk or make the array pass static or global.

Discussion of dynamic memory allocation:

You can create a duplicate copy of the string using malloc/calloc/realloc or strdup(which internally uses one of them) which has storage duration beyond the scope of it's declaration and return it. (One choice is dynamic memory allocation and using it for returning from the function).(The code for this is shown the later part of the answer)

Discussion of making the memory static or global.

Making it having a static storage duration is another choice. (Either make the array global or make it static in by explicitly using the static keyword.).

From §6.2.4p3 Under the point of static storage duration

... Its lifetime is the entire execution of the program and its stored value is initialized only once, prior to program startup.

What is the meaning of the last line? It's (the local variable's) lifetime is not going to end unless the program ends and that means we can return it from the function. (Yes this is a solution but you will not be seeing it - not a good way to do things).

Code for dynamic allocation

const char*p = "data";
char *s = malloc(strlen(p)+1);
if(!s){
   perror("malloc");
   exit(EXIT_FAILURE);
}
memcpy(s,p,strlen(p)+1);
return s;

And in main()

 char *p = text();
 if(p!=NULL)
     strcpy(saida,p);
 free(p);

Code for static storage duration examples:

In this case there is nothing much to show other than just make the pointer variable static.

static char palavar1[]="tata";
...
return palavar;

Without using all those difficult words - it can be said that simply now this local variable will stay alive even when the function ends. The previous problem discussed won't be a problem now. How long will it stay alive? Until the program ends. So you can safely use the above technique discarding the possibility of accessing object outside it's lifetime.

Other solutions

Other solution will simply pass the buffer which will be modified and the string literals content will be copied to it using strcpy or memcpy. There is no need to return something. This solution I didn't discuss at the first edit as it is a bit different approach to achieving what you want to do.

Standard directs that return type of main to be int not char. Ideally in your case int main(void) would do.

Community
  • 1
  • 1
user2736738
  • 28,590
  • 4
  • 37
  • 52
  • I will be glad if there is a way, you the respected Downvoter can tell me where it is wrong? – user2736738 Feb 05 '18 at 03:52
  • I did not vote down, but I can guess: Going into POSIX things is sort of off track. As long as there are good strictly C answers, it is good to stick with them. This answer does not mention the option of the caller providing a buffer to use. The voter might not have liked the phrasing and paragraph construction. The quote from 6.2.4 might not be a good way to teach about `static`. Teaching people can be hard. You want to go in small steps that connect to form a path from where they are to where they should go next. – Eric Postpischil Feb 05 '18 at 04:08
  • @EricPostpischil.: I am afraid I can't edit right now..I will get back to it when I come here. Thanks for the suggestion. – user2736738 Feb 05 '18 at 04:11
  • @EricPostpischil.: I got some time and edited a bit. I guess this will be good for dv retrieval atleast. WIll add more when time permits. Thanks. – user2736738 Feb 05 '18 at 08:57
0

You can make it static (and this is pointer of course):

static char palavra1[]= "tata"; 
סטנלי גרונן
  • 2,740
  • 21
  • 43
  • 62
purec
  • 198
  • 1
  • 6