1

Here are the codes of a program:

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

char * cloning(char * q){
    char s[strlen(q)];
    int i;
    for(i = 0; i < strlen(q); i++)
        s[i] = q[i];
    return s;
}

int main(){
    char q[] = "hello";
    char *s = cloning(q);
    return 0;
}

After the compilation a warning appears, so I changed the returned value like this:

char *b = s;
return b;

In this way the warning can be solved. However I found that inside the function cloning(), sizeof(s) is 5, but strlen(s) is 7. And if I change char s[strlen(q)] simply to char s[5], the output is still incorrect. Can anybody explain this problem to me? Thank you very much.

Improve this question
1
  • char * char_array; char_array = new char[150]; // delete [] char_array; char_array = NULL; Commented Oct 8, 2017 at 12:32

6 Answers 6

Reset to default
6

char s[strlen(q)] is a local variable, and hence when you return its address, it results in undefined behaviour. Thus either you could use strdup() or malloc() to dynamically allocate the array, thus ensuring that the array s is available on the heap when you return from the function. The returned array would need to be free()-ed as well, else it would have a memory leak :)

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

char * cloning(char * q){
    char *s = malloc(strlen(q)+1);
    // if you write char s[strlen(q)], it is defined locally, and thus on return gives an undefined behaviour
    int i;
    for(i = 0; i < strlen(q)+1; i++)
        s[i] = q[i];
    return s;
}

int main(){
    char q[] = "hello";
    char *s = cloning(q);
    free(s);
    return 0;
}
Improve this answer
Sign up to request clarification or add additional context in comments.

9 Comments

char s[strlen(q)]; is a valid C99 variable-length array; it is not undefined behavior. What is invalid, however, is returning a pointer to it because it has automatic storage duration.
The OP has not specified whether he is operating on C99, so I took some artistic license, so to speak :) But thanks for that bit. I edited the answer, to free the malloc()-ed array :)
If we were to assume they were using C89 rather than C99, the VLA usage would be a compile-time error. It would be caught by the compiler before you could even have a chance to invoke undefined behavior.
Ah yes. I forgot about that! To that point, I edited my answer to include Wumpus Q. Wumbley's point too :)
@CrazyHenry: The compiler calculates sizeof by looking at the definition of the array. strlen, on the other hand, will search through the array you pass it looking for the first NUL byte and returns the index of it. Since you're not NUL-terminating your string, strlen goes off the end of the array and triggers undefined behavior. char *b = s; return b; does nothing to make your program any more valid; it still triggers undefined behavior by returning a pointer to an object beyond its lifetime.
|
3 
char s[strlen(q)];

is a variable-length array. Like a malloc'ed buffer its size is determined at runtime. Unlike a malloc'ed buffer, it ceases to exist when the function returns.

Improve this answer

Comments

3

multiple issues with this code:

char * cloning(char * q){  
    char s[strlen(q)]; // s has strlen size but needs strlen + 1 to hold \0
    int i;
    for(i = 0; i < strlen(q); i++) // should copy the \0 otherwise q is not a valid string
        s[i] = q[i];
    return s;// returns the address of a local var == undef. behavior
}

if you want to clone a string just do strdup()

char* cloning(char* q)
{
  return strdup(q);
}

or the equivalent

char * cloning(char * q)
{
    char* s = malloc(strlen(q)+1);
    int i;
    for(i = 0; i < strlen(q)+1; i++)
        s[i] = q[i];
    return s;
}
Improve this answer

5 Comments

Actually you don't need to allocate strlen(q)+1 space for the array, because '\0' doen't need to be considered. This program can be corrected by changing the definition of char s[] to char *s = malloc(sizeof(char)*strlen(q)). My confusion is that when I change char s[strlen(q)] to char s[5] the result will be different.
@CrazyHenry: Using your existing code except replacing char s[strlen(q)]; with char s[5]; still causes undefined behavior by returning a pointer to an object with automatic storage duration and is thus invalid. Triggering undefined behavior doesn't have to be consistent.
@CrazyHenry If you want s to point to a string comparable equal to q, then s needs a '\0 in the same place as q.
@icktoofay Actually you can change the return value like this: char *b=s; return b; then the problem can be avoided. The program above is version 1.0. You can refer to my comment in the previous answer. Sorry I didn't declare the question very clearly.
@CrazyHenry: char *p = s; will make a new pointer, p, pointing to the object s. Then you return p. The lifetime of s ends, and now p is left pointing to an object after its lifetime has ended. You are still triggering undefined behavior.
2

The proper way to do this with standard C, no matter version of the C standard, is this:

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

char* cloning (const char* str)
{
  char*  clone;
  size_t size = strlen(str) + 1;

  clone = malloc(size);
  if(clone == NULL)
  {
    return NULL;
  }

  memcpy(clone, str, size); 

  return clone;
}

int main(){
    char original[] = "hello";
    char* clone = cloning(original);

    if(clone == NULL)
    {
      puts("Heap allocation failed.");
      return 0;
    }

    puts(clone);

    free(clone);
    return 0;
}
Improve this answer

1 Comment

@Wolf You'll have to add some error handling in main... ok I'll edit
0

Dynamic arrays in C are declared using Malloc and Calloc. Try googling it.

Eg:

 char *test;

    test = (char *)malloc(sizeof(char)*Multiply_By);
Improve this answer

1 Comment

Right, but does this answer the actual question?
-1

In C，static array is in stack,after function return,it's been destoryed. and string with char has a '\0' end. But strlen don't include it. For example.char q[] = "hello"; strlen(q) = 5,but the real size is 6 If you want to copy a string, the last '\0' must be added at the end.or using

char *s = malloc(sizeof(q)); ...; for(i = 0; i < sizeof(q); i++) s[i] = q[i];

you also need to free it after using.Maybe become a mem leak.

Hope this can help u.

Improve this answer

1 Comment

-1: Byd advice because sizeof will return 4. Just try it!

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.