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$
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
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),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,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,'\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,
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?,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),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.'\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.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,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.