0

I am currently writing a small dummy program to try and get the hang of properly using the read in c. I made a small function called readdata to read from the file descriptor and store in a buffer then return the number of bytes read. My problem is I am trying to correctly error handle and trap things so that there is no buffer overflow but I keep doing something from.

Here is the tester:

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

#define BUFSIZE 10

int readdata(int fd, char *buf, int bsize);

int main(void) {
   char buf[BUFSIZE];
   int returnval;
   int length;
   returnval = readdata(STDIN_FILENO, buf, BUFSIZE);
   printf("%s",buf);
   length = strlen(buf);
   fprintf(stderr,"The return value is %d\n", returnval);
   fprintf(stderr,"The string is %s\n",buf);
   fprintf(stderr,"The length of the string is %d\n",length);
   return 0;
}

Here is the small function:

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

int readdata(int fd, char *buf, int bufsize){
   int n = 0;
   if(fd < 0){
     return 1;
   }

   while((n=read(fd,buf,(bufsize-1)))>0){
      if(n == -1) {
         perror( "Read failed" );
         return 1;
      }
      else{
         buf[bufsize] = 0;
         return n;
      }
   }
}

If I run 

 cc -o test test.c readdata.c

And then put

echo "Hello" | ./test

It works fine. But if I pass the bufsize limit like this:

echo "1234567891" | ./getdatatest

It gives me this weird output where it says "the string is 123456789[some weird symbol]" . So I am not sure where to handle this error or why it is still incorrectly putting in the buffer when reading.

xuser23
  • 1
  • 2
    `buf[bufsize] = 0;` invokes undefined behaviour – too honest for this site Jan 23 '16 at 18:43
  • `read` and friends are usually used to read binary data, not text files. – Weather Vane Jan 23 '16 at 18:47
  • to start, `read()` is prototyped in unistd.h, which the posted code did not #include. Therefore, at best the compiler will assume all the parameters and the return type are `int`. Suggest inserting, at the top of the file containing `readdata()` the line: `#include ` then correct the call to `read()` to use the proper parameter and return types. To help you, the prototype is: `ssize_t read(int fd, void *buf, size_t count);` which strongly hints the `readdata()` function should be returning a `ssize_t` rather than an `int` – user3629249 Jan 24 '16 at 23:37
  • if there is a read() error then the loop is not entered, so the code block `if(n==-1)` will never be entered. Note: why return `1` when the read failed? that `1` will be seen by the caller as indicating one byte was read. Strongly suggest, on error, return -1 so caller is informed of the error. `read()` can return a `0` on two conditions. 1) when trying to read from a file that already at EOF and 2) when a signal is received. I.E. the code is not doing the right logic. – user3629249 Jan 24 '16 at 23:44
  • line: `buf[bufsize] = 0;` is writing past the end of the caller's buffer, resulting in undefined behaviour and can lead to a seg fault event. Also, a 0 (in memory as :0x00000000 (in a 32 bit architecture) is not a NUL byte of '\0'. Suggest replacing the `= 0` with `= '\0'` – user3629249 Jan 24 '16 at 23:58
  • while the `implicit conversion` feature will correct some of the problems with the posted code not using the correct variable/parameter/return types, it is much better to use the correct types and not depend on the compiler to fix your sloppy coding – user3629249 Jan 25 '16 at 00:01

1 Answers1

1

You do know that read() can return less characters than you requested? Also, buf[bufsize] is just past the end of buf. Your readdata function should also return something like -1 on error instead of 1 so you can distinguish the condition “one byte read” from “IO error.”

Consider something like this:

for (;;) {
    n = read(fd, buf, (bufsize - 1));

    if(n == -1) {
       perror( "Read failed" );
       return -1;
    } else {
       buf[n] = 0;
       return n;
    }
}
fuz
  • 88,405
  • 25
  • 200
  • 352
  • the while() loop is checking/exiting if the returned value is <=0,, so the `if` code block will never be entered. I.E. this answer leaves a LOT to be desired – user3629249 Jan 24 '16 at 23:52
  • @user3629249 Thank you for your remark. I improved the answer a little bit. – fuz Jan 25 '16 at 07:17