Troubles creating a get string function in C

Question

i'm trying to make a simple function that returns a string in a secure way, and i would like to know what's wrong with this code.
i decided to use read because both scanf and fgets gave e troubles, specifically, scanf gives me abort trap 6 if i overflow the buffer even with the lenght check obviously,
while fgets takes the input , returns the error message if the string i inserted was too long, but then preturn the string anyway without allowing me to reenter another string.
This is the code:

string get_string(const string prompt)
{
    char temp[30];
    int size,count;
    printf("%s",prompt);
    do
    {
        count=read(0,temp,29);
        temp[count]='\0';
        size=strlen(temp);
        if(size>24)
        {
            printf("\x1B[31mError\x1B[0m: too long.\n%s",prompt);
        }
        printf("size :%i\n",size);
    }
    while(size>24);
    stripc(temp,'\n'); //removes new line
    string word=malloc((size)*sizeof(char));
    strcpy(word,temp);
    return word;
}

Now with read it gives me kind of the same error, i read from stdin for a total of 30 bytes,
then i add null character at the end with the counter, but if the length exceeds this is the output:

ppppppppppppppppppppppppppppp
Error: too long.
size :30
size :2
p

Any idea what the problem is?
Is there another way to achieve what i need?

EDIT: The problem is not going out of range,
The strange thing is the fact that once i write more than 24 chars,
the function that should read input(read,scanf,fgets or whatever),
doesn't activate anymore, that's why size: appears two times in a row,
the input insertion is skipped for some reason and i would like to understand why.
i corrected some mistakes.

If you're doing the CS50 exercises then please add that as a tag. Otherwise people will start asking what the `string` type is (it's not a standard type, it's defined by the CS50 header file). — Some programmer dude, Jan 16 '18 at 13:46
i defined string in my library, when i use stripc the newline character will be erased, there will be a missing character so size is exactly the dimension that will accomodate the null character as well,i don't need to put size+1. — Adonai, Jan 16 '18 at 13:47
As for your problem, if you read 30 characters, then `read` will return `30`. Now, with your array of 30 elements, what is the range of valid indexes? Is `30` a valid index? — Some programmer dude, Jan 16 '18 at 13:48
`temp[30];` -> `temp[30 +1]`. Similarly you must malloc room for the null terminator. Yet another off-by-one bug, voting to close this. — Lundin, Jan 16 '18 at 13:50
nope, i put count-1 in the square bracket, the issue is the same. the fact is ,why when i get the error message the loop runs again but red doesn't ask for input? size : is written two times — Adonai, Jan 16 '18 at 13:51
You edited it and added `temp[count-1]='\0';` Do you realize that now truncates the last character you read? — Andrew Henle, Jan 16 '18 at 13:55
yes, the last charater i read is newline and i don't need that. the problem is that at the second iteration, when it should prompt read again, nothing happens and doesn't let me input again — Adonai, Jan 16 '18 at 13:57
Once answers start coming in, do not change the nature of your code - it makes for a moving target. Append if truly needed. Post rolled back. — chux - Reinstate Monica, Jan 16 '18 at 14:28
Output `size :30` only makes sense if prior UB occurred. Append a [MCVE] — chux - Reinstate Monica, Jan 16 '18 at 14:37
Note: `temp[count-1]='\0';` is insecure as `count < 1` is possible. — chux - Reinstate Monica, Jan 16 '18 at 14:38

user3121023 · Accepted Answer · 2018-01-16T18:00:46.043

if a newline is not found in the first input, call fgets until a newline is found so as to clean the input stream and then continue the outer while loop.

string get_string(const string prompt)
{
    char temp[26] = "";//24 + newline + '\0'
    int size,count;
    while ( 1)
    {
        printf ( "%s",prompt);
        if ( fgets ( temp, sizeof temp, stdin)) {
            if ( !strchr ( temp, '\n')) {//no newline
                printf("\x1B[31mError\x1B[0m: too long.\n%s",prompt);
                size = strlen ( temp);
                do {
                    fgets ( temp, sizeof temp, stdin);//read more and discard
                    size += strlen ( temp);
                } while (!strchr ( temp, '\n'));//loop until newline found
                printf("size :%i\n",size);
                continue;//re prompt
            }
            break;
        }
        else {
            fprintf ( stderr, "fgets problem\n");
            return NULL;
        }
    }

    temp[strcspn ( temp,"\n")] = '\0';//remove newline
    string word = malloc(strlen ( temp) + 1);
    strcpy ( word, temp);
    return word;
}

Thanks it works, but i would like to know the reason why the function to take input doesn't activate if i do it my way... can you explain? — Adonai, Jan 16 '18 at 14:22
so this happens because the \n that was written in the stdin the first time, but didn't end up in my string because of the limit upsets the next reading, right? — Adonai, Jan 16 '18 at 15:03
Should `fgets ( temp, sizeof temp, stdin)` return `NULL`, using `temp[]` is UB. With this code, perhaps an infinite loop. — chux - Reinstate Monica, Jan 16 '18 at 15:57

score -1 · Answer 2 · answered Jan 16 '18 at 14:22

-1

It is the same old error over and over again...

Strings in C are null terminated. That means the memory to hold the string must be one larger than the length of the string.

You check that the string read is not longer than 24 characters, but then in your malloc you do not add the room for the null character. Consequently, strcpy places the null character outside your alocated memory.

answered Jan 16 '18 at 14:22

Paul Ogilvie

25,048
4
23
41

The null termination have nothing to do with this probem, i put the +1 in my code but it doesn''t change the fact that the function that should read the phrase doens't activate at the second iteration of the loop. i know what null termination is – Adonai Jan 16 '18 at 14:24

Troubles creating a get string function in C

2 Answers2