C GOTO command creating infinite loop

Question

#include <stdio.h>

int main(void)
{
    char str[10];int i,length=0;
    line:
    { 
        printf("Enter string of length 10 \n");
        scanf("%[A-Za-z]s", str);
    }
    for (i=0; str[i] != '\0'; i++)
    {
        length++;
    }
    if (length != 10)
    {
        printf("Length of string is : %d \n Give a string of length 10 please \n", length);
        goto line;
    }
    else 
    {
        printf("length = %d \n", length);
        printf("Your String is: %s", str);
    }
    return 0;
}

If the user inputs a string of length not equal to 10, then my expected output should be : printing out "Enter string o length 10" and waiting for the user to input another string and then check it's length.

I have removed Goto statement and checked for string of length not equal to 10, it does compute the length and outputs it, something like this :

Enter string of length 10 
dciciibiyciyigiy
Length of string is : 16 
Give a string of length 10 please. 
*** stack smashing detected ***: ./palindrome.out terminated
Aborted (core dumped)

I have done similar Goto command execution in some other code elsewhere, but for this one : if I try to input a string of length not equal to 10, it shows the length and then goes into an infinite loop o printing something like this :

...
Length of string is : 428115 
Give a string of length 10 please. 
Enter string of length 10 
Length of string is : 428130 
Give a string of length 10 please. 
Enter string of length 10 
Length of string is : 428145 
Give a string of length 10 please. 
...

and so on. I do not understand what am I doing wrong?

Answer 1

To hold a 10-character string, str needs to be at least 11 elements wide:

char str[11]; // holds up to 10 characters plus 0 terminator

Unfortunately, the %s specifier doesn't know how big the target buffer is - if you enter more characters than the target buffer is sized to hold, those extra characters will be written past the end of the buffer, potentially clobbering important data. If you want to limit the number of characters read from the input stream into str, then you can add a field width to the format:

scanf( "%10s", str );

Unfortunately, unlike printf, you can't specify the field with as an argument - it must be hardcoded into the format string. There is a way around it using macros:

#define MAX_INPUT_LENGTH 10 // or whatever value
#define STR(x) #x           // "stringify" the argument           
#define EXP(x) STR(x)       // expand and then stringify the argument

#define FMT STR(%) EXP(MAX_INPUT_LENGTH) STR(s) // will expand to "%" "10" "s"

So you can write

char str[MAX_INPUT_LENGTH + 1]; // +1 for terminator
scanf( FMT, str );

and not worry about writing past end of str.

Now, about that goto...

There is a much better way to do this that doesn't use goto at all, and is a little easier to follow:

#include <string.h> // for strlen
...
char str[MAX_INPUT_LENGTH + 1];
size_t len = 0;
do
{
  printf( "Enter a string that's 10 characters long: " );
  scanf( FMT, str );
} while ( strlen( str ) != 10 );

Now, as written, this has a few problems. First, if you enter more than MAX_INPUT_LENGTH characters, the unread characters are left in the input stream, ready to foul up the next read. You would probably want to clear out any unread characters before asking for more:

do
{
  printf( "Enter a string that's 10 characters long: " );
  scanf( FMT, str );
  while ( getchar() != '\n' ) // consume unread characters up to the newline
    ; // empty loop 
} while ( strlen( str ) != 10 );

After scanf reads MAX_INPUT_LENGTH characters into str, it then scrubs any extra characters up to the newline character.

But there's one more problem - you won't get a friendly error message if the string length isn't right - you'll just get the original prompt again. If you want to write the error message, you'll need to do another check within the body of the do loop:

do
{
  printf( "Enter a string that's 10 characters long: " );
  scanf( FMT, str );
  while ( getchar() != '\n' )
    ; // empty loop
  if ( strlen( str ) != 10 )
    printf( "Input is not the right length, try again.\n" );
} while ( strlen( str ) != 10 );

Calling strlen twice is a bit ugly, so we can save the result of the first strlen call to use again. To use it as part of the loop control expression, it must be declared outside of the loop body:

size_t len = 0;
do
{
  printf( "Enter a string that's 10 characters long: " );
  scanf( FMT, str );
  while ( getchar() != '\n' )
    ; // empty loop
  if ( (len = strlen( str )) != 10 ) // assign as part of the check
    printf( "Input is not the right length, try again.\n" );
} while ( len != 10 );

Edit

Should have mentioned, you can avoid the buffer overflow concerns and macro trickery by using fgets instead of scanf:

size_t len = 0;
do
{
  printf( "Enter a string that's 10 characters long: " );
  if ( fgets( str, sizeof str, stdin ) )
  {
    while ( getchar() != '\n' )
      ; // empty loop
    if ( (len = strlen( str )) != 10 ) // assign as part of the check
      printf( "Input is not the right length, try again.\n" );
  }
  else
  {
    // EOF or error on input, handle as appropriate
  }
} while ( len != 10 );

To check str for non-alphabetic input, use the strpbrk function:

#define NON_ALPHA "0123456789!@#$%^&*()-_=+[]{};:,.<>/?'\\|\"";
...
size_t len = 0;
char *non_alpha_ptr = NULL;

do
{
  printf( "Enter a string that's 10 characters long: " );
  if ( fgets( str, sizeof str, stdin ) )
  {
    while ( getchar() != '\n' )
      ; // empty loop
    if ( (len = strlen( str )) != 10 ) // assign as part of the check
      printf( "Input is not the right length, try again.\n" );
    else if ( (non_alpha_ptr = strpbrk( str, NON_ALPHA ) ) )
      printf( "Input is not strictly alphabetic, try again.\n" );
  }
  else
  {
    // EOF or error on input, handle as appropriate
  }
} while ( len != 10 || non_alpha_ptr != NULL );

Answer 2

Most important problem with your program is that you are allocating memory for 10 char's (10 bytes) in str, but scanf is storing 11 char's (11 bytes) (10 char's which you enter + 1 sentinel/delimiter char '\0', that it inserts).

Hence the result of following line --

if (length != 10) 

can be true the first time even if you enter only 10 char's, and once it gets true for the first time, there is no looking back and length is only going to increase as there is nothing that decreases length in the goto loop. Hence the goto loop will loop forever.

Answer 3

You are not properly restricting how many characters the user can enter.

The array str can hold 10 bytes, but the call to scanf doesn't restrict the user from entering more characters. As a result, it is possible to write past the end of the array if too many characters are entered. This invokes undefined behavior, which in this case causes the code to crash.

You need to change the format specifier to scanf to allow a maximum of 9 characters (because you need to save 1 byte for the null terminating character):

scanf("%9[A-Za-z]", str);

You can then get rid of the length check (and the associated goto) since a string of 10 or longer can no longer be entered.

If you want the user to be able to enter up to 10 characters, you need increase the size of str to 11.