Consecutive scanf is adding up strings

Question

I've been having a problem with a very simple program and I really don't know why. There is a struct for a person:

typedef struct {
   char name[50];
   char p_id[11];
   char cel[11];
   int by;
   int id;
} Person;

Now, there is another struct which stands for the list of contacts:

typedef struct {
   Person * people;
} lContacts;

I've been trying to include the person's data to it, and add that person to the contact list. The person is being added normally, so I won't post the code here, but there is something wrong happening when I read the string:

void include(lContacts * myContacts)
{

    Person p;

    scanf("%s", p.name);
    scanf("%d", &p.by); //birth year
    scanf("%s", p.p_id);
    printf("TEST P_ID: %s\n\n", p.p_id);
    scanf("%s", p.cel);
    printf("TEST P_ID AGAIN: %s\n\n", p.p_id);

    myContacts->people[index]=p; //don't worry about the index, there is a piece of code I'm omitting to make it easier to read, just assume it is right.
    }
}

Notice that I have a print test there, because when I listed my contacts, the contact p_id had itself concatenated with the cel, so I printed the whole code until I found the mistake was there.

Here is a input example:

Name
1991
11111111111
<console prints| TEST P_ID: 11111111111>
22222222222
<console prints| TEST P_ID AGAIN: 1111111111122222222222>

however, if I print p.cel, it is correctly printed

<console prints 22222222222>

Any ideas? Maybe I should use '&' when scanning strings? (I read about it and the way I understood, there is no need.. is that right?)

Strings in C consist of an array of `char` terminated with the NUL character, aka `'\0'`. Since the `p_id` array is 11 bytes, you can only store 10 digits plus the NUL. In other words, you have a buffer overrun bug. — user3386109, Nov 09 '16 at 17:33
ie - change pid to char[12] (if 11 is the max length of pid) — pm100, Nov 09 '16 at 17:36

skrtbhtngr · Answer 1 · 2016-11-09T17:51:28.603

This is actually a very minute mistake here. Basically, you are experiencing a Buffer Overflow in the string p.p_id.

When you input the value for p.p_id as 11111111111 (11 times 1), you must realise that the actual capacity for the p_id string is declared as just 11 which must include the NULL character at the end.

If you assign 11111111111 to p_id there will be no space left for the NULL character in that string and so there will NOT be any at its end.

When you input the value for next member string of the structure, the same happens there, too.

Now, when you are trying to print the value of p.p_id the value will print until a '\0' (NULL character) is found in the string. But, there is none here, so the next string will start printing! (The buffer or string p_id overflows)

This is happening due to the member alignment done in structures in C. The next string will be stored in a consecutive memory cell, so the printing will continue. (If there was a third consecutive string member, then due to the buffer overflow in the second string, it will print too!)

But, the printing stops after second string because either there is no data in the next consecutive memory cell due to certain struct alignment or there may be an actual 0 value in the next cell which is interpreted as NULL character.

To avoid, either use a larger size for the character array or use dynamically allocated strings.

score 0 · Answer 2 · answered Nov 09 '16 at 18:14

As @skrtbhtngr pointed out, you have a buffer overflow. In order to prevent this in the future, you should use fgets instead of scanf on unknown string inputs. This example will exit(1) if the input is corrupted.

int getsafestring(char* s, int maxlen)
{
   if(fgets(s,maxlen,stdin) == NULL) return 1; // read error
   if(strlen(s) == 0) return 2; // other read error
   if(s[strlen(s)-1] !='\n') return 3; // buffer overflow error
   s[strlen(s)-1]=0; // replace newline with null
   return 0;
}

if(Getsafestring(p.name,sizeof(p.name))) exit(1);
if(scanf("%d", &p.by) != 1)              exit(1);
if(getsafestring(p.p_id,sizeof(p.p_id))) exit(1);
if(getsafestring(p.cel,sizeof(p.cel)))   exit(1);

Consecutive scanf is adding up strings

2 Answers2