Search code examples
cuser-input

C - write() repeats after reading user input with spaces


I'm creating a shell program with C and am working on getting user input after displaying the current working directory. I'm using read() and write() as I will need to use signals within my shell later on.

When I test my program, the prompt for input is duplicated whenever I give user input with spaces in it. The number of duplicates created is the number of spaces I give to my user input.

I'm assuming the problem is with my read() call but I'm unsure of how to solve it.

Here is the code:

#include "../include/msgs.h"
#include <linux/limits.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

const char *cwd_err_msg = FORMAT_MSG("getcwd", GETCWD_ERROR_MSG);
const chat *read_err_msg = FORMAT_MSG("read", READ_ERROR_MSG);

char *get_input();

int main() {

  while (1) {
    char cwd[PATH_MAX];

    // error checking
    if (getcwd(cwd, sizeof(cwd)) == NULL) {
      perror(cwd_err_msg);
    }

    strncat(cwd, "$", 1);
    write(STDOUT_FILENO, cwd, strlen(cwd));

    char *input = get_input();

    free(input);
  }

  return 0;
}

char *get_input() {
 char *input = (char *)malloc(100 * sizeof(char));

 if (read(STDIN_FILENO, input, sizeof(input)) == -1) {
   perror(read_err_msg);
 }

 return input;
}

And this is an example of what I see when I test the input:

/home/units/shell/build$test
/home/units/shell/build$testing and testing
/home/units/shell/build$/home/units/shell/build$/home/units/shell/build$

Solution

  • To begin there are a number of problems in the short getinput() function. There are also a number of additional cases you need to take into consideration, to protect against undefined behavior after taking input. A summary of the problems with the current getinput() function (non-exhaustive) are:

    Problems

    • as Ted Lyngmo points out your use of sizeof(input) does not provide the allocated size of input. This is because input is a pointer of type char*, which makes sizeof(input) actually sizeof(a_pointer). (generally 4 or 8 bytes depending on architecture),
    • while you generate an error with perror() when read() fails, you don't provide any way for the calling function to know whether input succeeded or failed. It is critical with every input function that you be able to convey to the caller whether the input was successful,
    • you never use the return of read() to determine how many characters (bytes) were actually read. This is critical to test whether the input exceeded the storage so you can detect when partial input was taken, and to know where the place the nul-terminating character,
    • and finally since you want to use what was read as input as a C-string, you must add the nul-terminating character '\0' (or just plain-old 0) to the end of what was read. To do so, you need to know where the end of input is. Making use of the return provides this information.

    Additional Consideration

    As noted in the other comments,

    • If you are dealing with small amounts of user-input, there is really no reason to repeatedly allocate and free with malloc(). Further, in C, there is no need to cast the return of malloc, it is unnecessary. See: Do I cast the result of malloc?,
    • A better approach would be to define a constant for MAXINPUT (or whatever name you choose), e.g. #define MAXINPUT 100 and then create a simple character array to use as a buffer for the input that you pass to getinput() for filling. By using a simple array, you avoid the need use malloc() altogether. (and with function stack space being 1M on windows and 4M on Linux by default, declaring a 1K buffer for user-input is well within reasonable size limits),
    • By passing a buffer to getinput() to fill, that allows you to change the return type to something that easily conveys whether the input succeeded or failed. Since read() already returns -1 on failure, or the number of characters read on success, you can simply change the return type to ssize_t and return the result of read(). This has the benefit of making the number of characters read available to the calling function.
    • You also need to decide what to do with the '\n' that will be the last character of the input. Normally you don't want input to have a dangling newline at the end. You can eliminate it after you nul-terminate the input by a simple call to strcspn() used as the index to overwrite the '\n' with the nul-character.
    • Additionally, you need to handle cases where the input exceeds the available storage. Not only do you read partial input, but everything that didn't fit is left in the input stream unread (just waiting to bite you on your next attempted input),
    • While you can certainly use read() and write() for user-input, if you are not constrained to do so, then using the higher-level input functions from stdio.h that operate on file-streams like fgets(), provides a much simpler way to handle line/character oriented input,
    • There are more considerations, like handling input when the user generates a manual EOF with Ctrl + d (or Ctrl + z on windows), etc.. which you can explore more fully later.

    Improving getinput()

    It doesn't take much to make your getinput() function a bit more robust and useful. If you fix all problems and take into account the listed considerations, you can come up with something like the following. This isn't to say this is the only or best way to solve all issues, but it is a generally robust solution that provides a return indicating success/failure to the caller, passes the buffer to fill as a parameter (and passes a prompt to display if non-NULL). It also validates that the input fits in the MAXINPUT size buffer with the required nul-terminating character and removes the trailing '\n', returning error and emptying the input stream if the input exceeds the storage available.

    #define MAXINPUT    1024      /* if you need a constant, #define one (or more)
                                   * size input buffer as needed, but DON'T SKIMP
                                   */
    
    ssize_t getinput (char *input, const char *prompt)
    {
      ssize_t n = -1;             /* no. of characters read or error */
      int maxinput_exceeded = 0;  /* flag indicating no space for nul-char */
      
      *input = 0;                 /* set input to empty string */
      
      if (prompt != NULL) { /* display prompt if not NULL */
        if ((n = write (STDOUT_FILENO, prompt, strlen (prompt))) == -1) {
          perror ("write-prompt");
          return n;
        }
      }
      
      /* read up to MAXINPUT bytes saving bytes read in n */
      if ((n = read (STDIN_FILENO, input, MAXINPUT)) == -1) {
        perror ("read-input");
        return n;
      }
      
      /* validate space remains to nul-terminate input as C-string or set error
       * flag and empty remaining characters from STDIN_FILENO.
       */
      while (n > 0 && input[n - 1] != '\n' && n == MAXINPUT) {
        maxinput_exceeded = 1;    /* set input exceeded flag */
        n = read (STDIN_FILENO, input, MAXINPUT);
      }
      
      if (maxinput_exceeded) {  /* if input exceeded return error */ 
        write (STDERR_FILENO, "error: input exceeds MAXINPUT\n",
               sizeof "error: input exceeds MAXINPUT\n" -1);
        
        return -1;  
      }
      
      /* nul-terminate input and strip trailing '\n' */
      input[n] = 0;
      input[(n = strcspn (input, "\n"))] = 0;
      
      return n;   /* return number of bytes in resulting string */
    }
    

    Putting together a short example that allows you to test a 10-character buffer by defining TEST10 as part of your compile string, you could do something line the following:

    #include <stdio.h>
    #include <string.h>
    #include <limits.h>
    #include <unistd.h>
    
    #ifdef TEST10
    #define MAXINPUT      10
    #else
    #define MAXINPUT    1024      /* if you need a constant, #define one (or more)
                                   * size input buffer as needed, but DON'T SKIMP
                                   */
    #endif
    
    ssize_t getinput (char *input, const char *prompt)
    {
      ssize_t n = -1;             /* no. of characters read or error */
      int maxinput_exceeded = 0;  /* flag indicating no space for nul-char */
      
      *input = 0;                 /* set input to empty string */
      
      if (prompt != NULL) { /* display prompt if not NULL */
        if ((n = write (STDOUT_FILENO, prompt, strlen (prompt))) == -1) {
          perror ("write-prompt");
          return n;
        }
      }
      
      /* read up to MAXINPUT bytes saving bytes read in n */
      if ((n = read (STDIN_FILENO, input, MAXINPUT)) == -1) {
        perror ("read-input");
        return n;
      }
      
      /* validate space remains to nul-terminate input as C-string or set error
       * flag and empty remaining characters from STDIN_FILENO.
       */
      while (n > 0 && input[n - 1] != '\n' && n == MAXINPUT) {
        maxinput_exceeded = 1;    /* set input exceeded flag */
        n = read (STDIN_FILENO, input, MAXINPUT);
      }
      
      if (maxinput_exceeded) {  /* if input exceeded return error */ 
        write (STDERR_FILENO, "error: input exceeds MAXINPUT\n",
               sizeof "error: input exceeds MAXINPUT\n" -1);
        
        return -1;  
      }
      
      /* nul-terminate input and strip trailing '\n' */
      input[n] = 0;
      input[(n = strcspn (input, "\n"))] = 0;
      
      return n;   /* return number of bytes in resulting string */
    }
    
    int main (void) {
    
      char buf[MAXINPUT] = "";
      ssize_t n = 0;
      
      /* while characters other than '\n' (empty line) read */
      while ((n = getinput (buf, "input: ")) > 0) {
        write (STDOUT_FILENO, buf, n);
        write (STDOUT_FILENO, "\n\n", 2);
      }
      
    }
    

    (note: you simply need to press Enter alone at the input prompt to end the program)

    Example Use/Output

    With a sufficient 1024 byte buffer:

    $ ./bin/readwrite-getinput
    input: my dog has fleas
    my dog has fleas
    
    input: my cat has none
    my cat has none
    
    input: lucky cat :)
    lucky cat :)
    
    input:
    

    Using a compile string for gcc with full warnings enabled and TEST10 defined to test input that exceeds the buffer size, you can compile as follows:

    $ gcc -Wall -Wextra -pedantic -Wshadow -Werror -std=c11 -Ofast -DTEST10 -o readwrite-getinput readwrite-getinput.c
    

    Now exercising the input routine, which provides an error message and exits if input exceeds the storage avaialble:

    $ ./readwrite-getinput
    input: 123456789
    123456789
    
    input: 1234567890
    error: input exceeds MAXINPUT
    

    Look things over and let me know if you have questions.