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

Question

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$

You never check how many characters read actually read. You should. — Ted Lyngmo
– Ted Lyngmo, Commented Sep 29, 2024 at 20:29
Also: sizeof(input) will give you the size of a char*. It'll most likely be 4 or 8, not 100. — Ted Lyngmo
– Ted Lyngmo, Commented Sep 29, 2024 at 20:30
And strncat(cwd, "$", 1); always adds to the string even if there's not enough space. — Andrew Henle
– Andrew Henle, Commented Sep 29, 2024 at 20:41
In C, there is no need to cast the return of malloc, it is unnecessary. See: Do I cast the result of malloc? E.g. char *input = malloc (100); Where sizeof(char) is always 1, so it is superfluous. Better, #define MAXINPUT 100 and then char *input = malloc (MAXINPUT); (avoid using Magic Numbers) — David C. Rankin
– David C. Rankin, Commented Sep 29, 2024 at 20:41
strncat(cwd, "$", 1); will replace the terminal NUL of cwd with a $ and NOT append another NUL. So the result will not be NUL terminated and you get undefined behavior when you go and call strlen on it. There are probably more NULs somewhere in the buffer, but you may well get garbage as well. — Chris Dodd
– Chris Dodd, Commented Sep 30, 2024 at 0:44

David C. Rankin · Accepted Answer · 2024-09-30 00:45:10Z

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.

Collectives™ on Stack Overflow

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

1 Answer 1

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Related