(C) scanf not working when the terminate value is [^\n]

Question

I am creating a program where you can execute terminal commands via the program. I want to access a directory (using cd /Users/user/Desktop) but since scanf terminates at a whitespace, I was forced to change the terminate value to [^\n]. That's when the error popped up. Whenever I enter in a command, it executes said command, but then the next time the program goes through the (infinite) loop, it doesn't stop executing the line before the scanf function. This hasn't happened when the terminate value was %s. Here is the code for the program:

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

void execute(){
    char* command = (char *) malloc(15);
    char* output = (char *) malloc(4096);
    printf(">> ");
    scanf("%[^\n]", command);            //Here is the scanf function
    FILE* cmd = popen(command, "r");
    fread(output, sizeof(output), 32000, cmd);
    if (strlen(output) != 0){
        printf("\n%s\n", output);
    }
    free(output);
    pclose(cmd);
}

int main(){
    while (1){
        execute();
    }
}

Here is the output when the terminate value is [^\n]:

>> ls

Applications
Desktop
Documents
//Here the rest of the contents in my user folder appear (twice for some reason, that's also an issue related to this).

>> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> 
>> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> 
>> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> 
>> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> 
// This then goes on forever

and here is the output when the terminate value is %s:

>> ls

Applications
Desktop
Documents
//Here the rest of the contents in my user folder appear (once)

>> //Here I can input things again

Could someone show me how to fix this? (I tried gets(), same outcome)

Serket, do not append your fix here. You can post it as you own answer below. Post rolled-back. — chux - Reinstate Monica, Apr 11 '20 at 22:40

Joseph Sible-Reinstate Monica · Accepted Answer · 2020-04-11T23:15:49.963

Since you're telling scanf not to read the \n, it's leaving it in stdin, so when the loop repeats, it's still there and causes the next iteration to immediately return an empty string. To fix it, a few choices:

Change "%[^\n]" to " %[^\n]" to ignore leading whitespace.
Add getchar(); after scanf("%[^\n]", command); to consume the newline.
Use fgets or some other reading function other than scanf. (Not gets; it can't be used safely!)

You're also not checking whether scanf returned anything, so if it failed to parse anything at all, then you're passing uninitialized memory to popen.

Side note: there's lots of other bugs in your program, but they're not relevant to your immediate problem:

You leak the memory of command each time through your loop. To fix it, add free(command); somewhere after the popen and before the end of the function.
You didn't pass a maximum field width to scanf, so if you enter more than 14 characters, you'll have a buffer overflow and corrupt memory. To fix it, change %[^\n] to %14[^\n] (14 and not 15 so there's room for the null terminator). You should also detect the case of a partial read and handle it properly, to avoid echo Their alarm is set from deleting files called is and set, for example.
sizeof(output) will be the size of the pointer, not of the memory you allocated that it points to, and 32000 seems to have come out of thin air. Change sizeof(output) to 1 and 32000 to 4096.
fread doesn't null-terminate its output, so printing it with %s will print uninitialized memory after it. To fix it, either use something other than fread to get the output, or use its return value and make sure you only print that many characters.

With your "fixed" version, there's still a few more problems:

Your first fgets won't buffer-overflow now, but overly-long strings will still cause problems. In particular, if a user enters a command that's too long, it will act as if they hit Enter in the middle of it, resulting in two partial commands being ran.
Doing cd in a subprocess doesn't affect the parent process or any other subprocesses. To make it work, you'd need to check to see if the command they entered starts with cd, and if so, call chdir directly instead of doing popen.

Here's a way that works right:

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

int main(){
    char *command = NULL;
    size_t commandsize = 0;
    char *output = (char *)malloc(5000);
    while(1){
        printf(">> ");
        ssize_t commandlen = getline(&command, &commandsize, stdin);
        if(commandlen < 0) {
            // assume EOF. Small chance that it was an error though
            break;
        }
        printf("%s", command);
        if(!strcmp(command, "exit\n")) {
            break;
        }
        if(!strncmp(command, "cd ", 3)) {
            command[commandlen - 1] = '\0'; // remove the newline
            if(chdir(command + 3)) {
                perror("chdir");
            }
            continue;
        }
        FILE *cmd = popen(command, "r");
        if(!cmd) {
            perror("popen");
            continue;
        }
        size_t sz;
        while ((sz = fread(output, 1, 5000, cmd)) > 0){
            fwrite(output, 1, sz, stdout);
        }
        pclose(cmd);
    }
    free(command);
    free(output);
    return 0;
}

A few notes about it:

Instead of malloc and freeing stuff every time through the loop, I moved those variables up to function-scope so it only happens once the whole time the program's running.
I use getline to read the whole line, and automatically allocate however much space is necessary. This, like popen, isn't part of standard C but is part of POSIX.
I use fread and fwrite to shuttle data from the process we started, to avoid having to think about null terminators. I assume you're eventually going to do some sort of processing on this data; if not, consider using system instead of popen, which will automatically write the output back to the user.

Thanks. Could you point out the other bugs to me as I'm relatively new to C? — Serket, Apr 11 '20 at 22:06
@Serket don't use scanf, don't forget to dealloc(free) a memory field allocated already. — Soner from The Ottoman Empire, Apr 11 '20 at 22:08
I have a new issue that I would really appreciate if you could help me with: For some reason, I cannot change directory (cd). Why is that? I've added the new code to the question. — Serket, Apr 11 '20 at 22:35
@JosephSible-ReinstateMonica I have posted the fixed code as an answer, could you modify it so the ```cd``` command would work (the answer has been removed from the question)? — Serket, Apr 11 '20 at 22:42
@JosephSible-ReinstateMonica How should I fix the command length issue? (I could just increase the maximum command length, is that the solution?) — Serket, Apr 11 '20 at 22:46
@Serket No, it isn't. You can't make it infinitely long, so no matter how long you make it, someone else could just enter something longer. Another edit coming up... — Joseph Sible-Reinstate Monica, Apr 11 '20 at 22:47

Serket · Answer 2 · 2020-04-11T23:43:08.457

-1

Here is the fixed code (other than the cd issue) (Another user has posted the final version of the code. Their answer has been marked as the correct answer). IGNORE THE FOLLOWING CODE, IT IS NO LONGER RELEVANT:

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

int main(){

    while(1){
        char* command = (char *) malloc(30);
        char* output = (char *)  malloc(5000);
        printf(">> ");
        fgets(command, 30, stdin);
        printf("%s", command);
        FILE *cmd = popen(command, "r");
        while (fgets(output, 5000, cmd) != NULL){
            printf("%s", output);
        }
        pclose(cmd);
        free(command);
        free(output);
    }
    return 0;
}

edited Apr 11 '20 at 23:43

answered Apr 11 '20 at 22:41

Serket

3,785
3
14
45

I suspect the `'\n'` in `command` causes trouble with `popen(command, "r");` See https://stackoverflow.com/q/2693776/2410359 – chux - Reinstate Monica Apr 11 '20 at 22:44
@chux-ReinstateMonica I don't think it will. `popen` passes its argument to `/bin/sh -c`, and the shell should just swallow that extra `\n`. – Joseph Sible-Reinstate Monica Apr 11 '20 at 22:46
1

@JosephSible-ReinstateMonica Perhaps so. IDK. Code should check the return value of `popen()` before using it in `fgets()` thought, in case things did not work. – chux - Reinstate Monica Apr 11 '20 at 22:48

(C) scanf not working when the terminate value is [^\n]

2 Answers2