Coding a check for array overflow? or how to have no input limit.

Hi guys,

I'm about 4 weeks in to a beginners C course I'm taking online, the information is being given but it seems they never really touch more than the surface.

currently they are saying getting a string is an issue because of array size, and overflowing this causes segmentation faults.

they havent said how to get around this so I've been trying to figure it out as per my code below :

Code:

---

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

int main(void)
{
//array with 20 char spaces
char s[20];

do
    // take user input including white space
    {
    scanf("%[^\n]",s);
    }
// while s is empty
while ( s == NULL);

// convert s to an int so I can check its not > array size
int count = atoi(s);

if (count > 20)
    {
    // return erro code
    return 1;
    }

else
    {
    // print input to screen
    printf("%s\n",s);
    }

return 0;
}

---

unsure as to why this isnt working, should I be using a linked list for an input string? we have only just started to look at linked lists.

please help, thanks.

Hi,

Based on the line 7, of your example

Code:

---

char s[20];

---

The max length is known.

Also I don't understand the use of atoi in your code. You find the length of the string by using strlen (see below)

When you know how many characters you can read (at most), you can pass this information to scanf. Here is an example that reads at most 5 characters:

Code:

---

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

int main(int argc, char *argv[])
{
    char text[5];
   
   
    scanf("%5s",text);
   
    printf("The text you entered is: %s\n", text);
    printf("The length of your text = %zu\n", strlen(text));
   
    return EXIT_SUCCESS;
}

---

Quote:

---

Originally Posted by MonkeyTennis

they havent said how to get around this so I've been trying to figure it out as per my code below

---

Have you learnt about pointers and how to use malloc and free?

dariyoosh: since you declared text to be an array of 5 char, the format for scanf should be "%4s".

You have many problems here.

You don't initialize s. It will pick up garbage in memory.
You are not capturing the return value from scanf(), to check if scanf() actually returned a valid string.

Code:

---

while ( s == NULL);

---

s will NEVER be NULL! s is always the address of the first char in the array!

Code:

---

int count = atoi(s);

---

atoi retuns the string in s converted to an integer, NOT the length of the string entered. If I typed in "123" then count would be the integer 123, not 3!
You need to use strlen(), NOT atoi().

Code:

---

if (count > 20)

---

If I had typed in "123" then the if statement would be true. You are trying to test if the user typed in more than 20 characters, but by then, if I had, you would have overwritten beyond the end of the array!
There is a way to do it correctly with scanf() but better to use fgets() to input a string of maximum 19 characters, allowing room for a Nul byte, '\0'.

Quote:

---

Originally Posted by laserlight

dariyoosh: since you declared text to be an array of 5 char, the format for scanf should be "%4s".

---

According to man 3 scanf, I don't think that 4 is the right value but rather 5

Quote:

---

...
An optional decimal integer which specifies the maximum field width. Reading of characters stops either when this maximum is reached or when a nonmatching character is found, whichever happens first. Most
conversions discard initial white space characters (the exceptions are noted below), and these discarded characters don't count toward the maximum field width. String input conversions store a terminating
null byte ('\0') to mark the end of the input; the maximum field width does not include this terminator.
...

---

And here is the output of the example I provided:

Code:

---

$ gcc -Wall testprog.c -o testprog
$ ./testprog
hellooooooooooooooooo
The text you entered is: hello
The length of your text = 5
$

---

Quote:

---

The text you entered is: hello
The length of your text = 5

---

The null byte was stored OUTSIDE the end of the array, NOT inside at text[4], the last byte in the array.

Quote:

---

According to man 3 scanf, I don't think that 4 is the right value but rather 5

---

No, look closely at that quote you provided, the part you highlighted is the key, along with this:

Quote:

---

Reading of characters stops either when this maximum is reached

---

You must reserve room for the end of string character, which scanf() always adds to a string.

Jim

Quote:

---

Originally Posted by dariyoosh

According to man 3 scanf, I don't think that 4 is the right value but rather 5

---

No, 4 is the right value. Read carefully the part that you bolded: if the field width specified does not include the null terminator, then we have to account for the null terminator. So, if you specify the field width as 5, then a maximum of 5 characters could be read into the array. So, with the null terminator to make it a null terminated string, this means that the array must have a minimum of 6 characters. But the array was declared as having 5 characters, hence a buffer overflow becomes possible.

Quote:

---

Originally Posted by jimblumberg

No, look closely at that quote you provided, the part you highlighted is the key, along with this:

You must reserve room for the end of string character, which scanf() always adds to a string.

Jim

---

I see now my mistake, thanks for the explanation.

Quote:

---

Originally Posted by laserlight

No, 4 is the right value. Read carefully the part that you bolded: if the field width specified does not include the null terminator, then we have to account for the null terminator. So, if you specify the field width as 5, then a maximum of 5 characters could be read into the array. So, with the null terminator to make it a null terminated string, this means that the array must have a minimum of 6 characters. But the array was declared as having 5 characters, hence a buffer overflow becomes possible.

---

Thanks for pointing this out.

Hi guys, thanks for pointing these things out to me, cant believe I tried to use atoi and not bloody strlen!

Yes we have been fought pointers, however I understand how they work but not how to use them properly, that's the problem with cs50 through edx, its very disjointed. Should I be allocating s with a pointer and malloc before setting input to s?

We had "training wheels" on up untill this week which handled scanf and everything else, all we had to put was getstring() and it would handle all the allocation etc, I'm struggling to implement simple things now.

For a program this simple, I would not bother with using malloc() and free().

Unfortunately, a course that provides "A broad and robust understanding of computer science and programming" cannot teach you a thorough understanding of the C Language, that you would need to properly use the language. This also goes for "PHP, and JavaScript plus SQL, CSS, and HTML" that the course introduces to the student. You would need to follow up with a full course in C and/or any other language you wish to learn.

is this better? is this all I have to do every time I need to store user input?

Code:

---

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

int main(void)
{
//array with 50 char spaces
char* s = malloc(50 * sizeof(char));

do
    // take user input including white space
    {
    printf("please enter some text no longer than 50 chars:\n");
    scanf("%[^\n]",s);
    }
   
// while s is empty or over 50
while ( strlen(s)+1 == 0 || strlen(s)+1 > 50 );

if (s != NULL)
    {
    // print input to screen
    printf("\nYou typed:\n");
    printf("%s\n",s);
    }
   
else
    {
    free(s);
    return 1;
    } 
   
free(s);
return 0;
}

---

Why the malloc? Why not just use a statically allocated array?

Never use a function that doesn't limit the number of characters it will try to retrieve into a string. When using scanf() always use the "optional" width specifier to limit the number of characters.

Look at the following:

Code:

---


//array with 50 char spaces
char* s = malloc(50 * sizeof(char));
...
  {
    printf("please enter some text no longer than 50 chars:\n");
    scanf("%[^\n]",s);

---

Your comment and the printf is incorrect. Since the size of the array is 50 you can enter at most 49 characters into your string. You must always reserve room for the end of string character ('\0'). Your scanf() should look like:

Code:

---

  scanf("%49[^\n", s);

---

Although in this case I would probably suggest using fgets() instead.

Code:

---

fgets(s, 50, stdin);

---

Jim

The use of malloc() really does not help you here. Many of the problems still exist in the code.

Code:

---

char* s = malloc(50 * sizeof(char));

---

"sizeof(char)" is ALWAYS 1, and only 1.

Code:

---

char* s = malloc(50);

---

You should also check if the return from malloc() was successful.

Code:

---

do
    // take user input including white space
    {
    printf("please enter some text no longer than 50 chars:\n");
    scanf("%[^\n]",s);
    }
   
// while s is empty or over 50
while ( strlen(s)+1 == 0 || strlen(s)+1 > 50 );

---

If the user has typed in more than 49 characters, you're too late! You have overwritten memory outside of the allocated memory!
A better solution:

Code:

---

scanf("%49[^\n]", s);

---

Or better use fgets().
"strlen(s)+1 == 0" strlen() returns 0 or greater than 0, "strlen(s)+1" will NEVER equal zero!

Code:

---

if (s != NULL)
    {
    // print input to screen
    printf("\nYou typed:\n");
    printf("%s\n",s);
    }

---

You are not checking if someone typed in a string successfully, you are only checking that malloc() returned a valid address of the allocated memory!

I think you need to pick up a good book on C that can fill in the gaps that the course left WIDE open.