0

I have the following C struct and using the function getPerson(void) that returns a pointer to my struct returns a pointer to a new struct using user input. The following code does not compile and it gives the following error:

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

     typedef struct {

         char name[50];
         int age;

     } person;

     person* getPerson(void) 
     {   
         person* newPerson = (person*)malloc(sizeof(person));
         int ageInput;
         char *nameInputPtr = (char*)malloc(50 * sizeof(char));

         printf("Please enter your name: \n");
         scanf("%s", nameInputPtr);
         printf("Please enter your age: \n");
         scanf("%d", &ageInput);

         newPerson->name[50] = *nameInputPtr;
         newPerson->age = ageInput;

         return newPerson;  
   }

Error I get:

struct.c:22:2: error: array index 50 is past the end of the array (which contains 50 elements)
  [-Werror,-Warray-bounds]
    newPerson->name[50] = *nameInputPtr;
    ^               ~~
struct.c:6:2: note: array 'name' declared here
    char name[50];
    ^

I manage to fix my error by the following change in the line 22:

22  newPerson->name[49] = *nameInputPtr;

So my change was to change number 50, to number 49 to be inside the bounds of the index definition of line 6.

Therefore I don't understand why line 6 and line 22 give an error in my original code and I would like to have an explanation about the error and on the clarity and functionality of my solution to this.

Natasha Dutta
  • 3,242
  • 21
  • 23
hack-is-art
  • 325
  • 5
  • 20
  • C arrays are indexed from zero, so last valid index in array[50] is 49. For analogy, array[10] has ten valid indexes: 0123456789. – user3125367 Jun 05 '15 at 12:16
  • arrays in most programming languages are 0 indexed, thus index in-bounds would be between 0 and n-1 – Ivaylo Strandjev Jun 05 '15 at 12:16
  • [do not cast](http://stackoverflow.com/q/605845/2173917) the return value of `malloc()`, as said by @unwind. :-) – Natasha Dutta Jun 05 '15 at 12:35
  • 1
    You could simplify your code a lot by reading directly into `newPerson`'s fields, and not have the two temporary variables `ageInput` and `nameInputPtr` – M.M Jun 05 '15 at 12:36
  • 1) when calling scanf() always check the returned value (not the parameter value) to assure the operation was successful. 2) using "%s" allows the user to overflow the receiving buffer. suggest 'scanf("%49s", nameInputPtr);' where the '49' is one less than the length of the input buffer, as scanf will append a '\0' character to a %s input. (note, there are other ways to specify the max length of a %s input besides the suggested method) – user3629249 Jun 06 '15 at 16:58
  • 1) in C, do not cast the returned value from malloc() and family of functions. 2) always check (!=NULL) the returned value from malloc() and family of functions to assure the operation was successful 3) 'sizeof(char)' is always 1, so avoid cluttering the code by not using that expression – user3629249 Jun 06 '15 at 17:03
  • this line: 'newPerson->name[50] = *nameInputPtr;' will NOT copy the string. suggest using: 'strcpy( newPerson->name, nameInputPtr);' – user3629249 Jun 06 '15 at 17:05

3 Answers3

3

Array index in C is 0 based. For an array with 50 bytes of memory allocated,

 char name[50];

trying to use [50] as index is off-by-one and invokes undefined behaviour.

That said,

 newPerson->name[50] = *nameInputPtr;

is not the way you copy a string. You need to make use of strcpy(), like

strcpy(newPerson->name, nameInputPtr);

Also, it is a good practice to limit the input string length while using scanf() to avoid possible buffer overrun. Change

scanf("%s", nameInputPtr);

to

scanf("%49s", nameInputPtr);

However, please keep in mind, there is not much point in using dynamic memory if you already have a design with fixed-sized allocation. You can vary easily make use of a compile-time allocated array.

Natasha Dutta
  • 3,242
  • 21
  • 23
2

What?

This:

newPerson->name[50] = *nameInputPtr;

says "assign the character at *nameInputPtr to the character at index 50 in name". But name is only 50 characters long, and arrays are 0-based in C so this is out of bounds.

Still, that code doesn't make any sense! You want:

strcpy(newPerson->name, nameInputPtr);

to copy the entire string. This runs the risk of propagating a buffer overrun since you don't limit the input in scanf(), though.

So, better, since you already have a person, just input into it:

scanf("%49s", person->name);

Remember to check the return value.

Of course you should do the same for the age, no need for a separate integer which is then copied into the structure.

unwind
  • 391,730
  • 64
  • 469
  • 606
  • And/or use `strncpy()` – Eregrith Jun 05 '15 at 12:23
  • @Eregrith I never recommend `strncpy()`, due to it being weird and strange and generally dangerous. – unwind Jun 05 '15 at 12:27
  • Strange and dangerous? I didn't know that ... In what context ? – Eregrith Jun 05 '15 at 12:30
  • 1
    @Eregrith It's strange, because it zero-pads the destination up to the specified length. Nobody wants that for strings. It's dangerous, since it doesn't terminate the destination if the source is longer than the destination size. Nobody wants that. Hence, I don't recommend it. – unwind Jun 05 '15 at 12:31
  • it only zero pads if your source is too small which should never happen if you use `strncpy` properly in my opinion. On the other hand, not terminating the destination is quite a problem indeed. I forgot about that – Eregrith Jun 05 '15 at 12:34
  • @Eregrith "Too small"? So in this case, if my name is shorter than 49 characters it wouldn't be right to use `strncpy()`? – unwind Jun 05 '15 at 12:35
  • @unwind how did you miss the casting part?? :-) – Natasha Dutta Jun 05 '15 at 12:36
  • `strncpy(buf, name, MIN(strlen(name), bufsize - 1))` + last `\0`. How hard is that? – Eregrith Jun 05 '15 at 12:36
  • @Eregrith the strncpy never null terminates so you may as well use `memcpy`. (Which is still worse than `strcpy` in this case). – M.M Jun 05 '15 at 12:37
  • How is `strcpy` *less* dangerous than `strncpy` correctly clamped? If you ever change your `scanf` and forget about the copy you're in trouble – Eregrith Jun 05 '15 at 12:42
  • @Eregrith I not always clamp strncpy with strlen - But when I do, I memcpy and set \0 by hand instead. – user3125367 Jun 05 '15 at 12:57
  • @Eregrith Not too hard I guess if you've understood all the reasoning. Of course `MIN()` is non-standard, and the complexity of that compared to the expected usecase of `copy(dest, source, sizeof dest);` is just horrendous, in my opinion. Even having to call `strlen()` for an operation like this is insane, in my opinion. – unwind Jun 05 '15 at 12:57
  • `MIN` in this case is simply a macro with a ternary condition such as `#define MIN(x, y) ((x) < (y)) ? (x) : (y)` The complexity can be reduced if you use the number of read chars from your input reading function's return instead of strlen. – Eregrith Jun 05 '15 at 13:00
  • And it does not change the fact that `strcpy` is not stopping at the end of the buffer so I can't see how it could be safer – Eregrith Jun 05 '15 at 13:01
  • 1
    @Eregrith strncpy is a bad name for completely different fixed-buffer oriented routine that designed to be used in very specific cases, almost unpresented now. I can't even think of modern data-processing implementations that cut strings by some arbitrary limit and that is ok for business logic. Otoh, some older fixed-field-width dbases may use strncpy along with printf("%.50s") (iirc format) as-is. – user3125367 Jun 05 '15 at 13:05
  • @Eregrith if you change your buffer size and forget about the strncpy you're also in trouble... a better way would be to just read directly into the target buffer and avoid this issue entirely! – M.M Jun 05 '15 at 13:50
  • @MattMcNabb Yes. But usually I am not because the buffer size is *also* a `#define` value. My previous example was not fully defined I agree. – Eregrith Jun 05 '15 at 13:52
0

You have to use sprintf

strcpy(newPerson->name, nameInputPtr);

Take note that arrays in C are 0 based, the name[50] does not exist.

In your specific case you are using array as string, the you must verify that the size of your array is STR_LEN_MAX+1, because of strings are null terminated. That means strings always need a byte after last char in your string where a '\0' char can be inserted.

LPs
  • 16,045
  • 8
  • 30
  • 61
  • `strcpy` would be a lot simpler here. The source has already been verified to not be too big. Also your code would cause an exploit if the person typed `%` codes into their input. – M.M Jun 05 '15 at 12:35
  • See discussion on unwind's answer for the problems with `strncpy`; it's not a drop-in replacement for `strcpy` – M.M Jun 05 '15 at 12:47
  • @MattMcNabb Thanks. I never heard that strncpy do not insert null termination if `strlen(src) >= defined_max_len`. I'm not use to code with input from users. – LPs Jun 05 '15 at 12:52