read() - Check buffer boundaries if used in a loop including recursive loops

Question

I have this code and run it with Flawinder, and i get this output on the read() functions:

Check buffer boundaries if used in a loop including recursive loops

Can anyone see the problem?

#include <stdlib.h>
void func(int fd)
{

char *buf;
size_t len;
read(fd, &len, sizeof(len));

if (len > 1024)
return;
buf = malloc(len+1); 
read(fd, buf, len); 
buf[len] = '\0';
}

I'd be more concerned about how this completely ignores potential errors returned from `read` *both* times (and `malloc`). The first is especially important, as it could leave `len` *indeterminate*. Don't violate [Spencer's Sixth Commandment](http://www.seebs.net/c/10com.html); check your IO results for errors. — WhozCraig, Oct 27 '18 at 09:09

score 4 · Accepted Answer · edited Oct 29 '18 at 16:33

4

you should check the return value of read() to know whether call to read() was success or failure or if read() was interrupted by a signal then set the errno. For e.g

ssize_t ret = read(fd, &len, sizeof len);
if( (ret == -1 || ret != sizeof len) {
   /* error handling @TODO */
}

Most importantly here

ret = read(fd, buf, len); /* read() may read less than len characters */

read() returns the number of bytes read, so instead of this

buf[len] = '\0';

use

buf[ret] = '\0'; /* correct way */

Sample Code

void func(int fd) { /* assume fd is a valid file descriptor */
        char *buf = NULL;
        size_t len;
        errno = 0; /* set this to 0 */
        ssize_t ret = read(fd, &len, sizeof len);
        if( (ret == -1 || ret != sizeof len) {
                /* error handling @TODO */
        }
        if (len > 1024) {
                return;
        }
        buf = malloc(len+1); 
        if(buf == NULL) {
                /* error handling @TODO */
        }
        ret = read(fd, buf, len);
        if(ret!=-1) {
                buf[ret] = '\0';
                /* do something with buf and free it once usage is done*/
        }       free(buf); /* free the buf */
        else { /* if read failed */
                free(buf); /* free the buf */
        }
}

edited Oct 29 '18 at 16:33

alk

69,737
10
105
255

answered Oct 27 '18 at 09:20

Achal

11,821
2
15
37

1

`read()` might very well return less then it was told to do. Which at least in the 1st case would be fatal. So testing for `-1` only is not sufficient. – alk Oct 27 '18 at 09:23
1

Also `read()` returns `ssize_t` not `int`, if not on Windows. – alk Oct 27 '18 at 09:28
@alk I have edited about 1st case to check for `errno`. – Achal Oct 27 '18 at 09:32
This `if(ret == -1 && errno != 0) {` should be `if(ret == -1 || ret != sizeof len) {`. No need to test `errno` for `!= 0` if `read()` returned `-1`. – alk Oct 27 '18 at 09:33
1

Thinking twice `if (ret != sizeof len) {` would do. Still in any case of `ret > 0 && ret < sizeof len` one could loop around `read()` reading until `sizeof len` bytes were read. One could also do this in case of `ret == -1 && (errno == EAGAIN || errno == EINTR)`. – alk Oct 27 '18 at 09:39
1

I also observed rare cases where even `ret == -1 && errno == EIO` was worth a retry. This typically happened when reading from network based file systems. – alk Oct 27 '18 at 09:45
@alk Do you know what this line does in the code? ** if (len > 1024) { return; } ** – Per Kristian Gravdal Oct 29 '18 at 12:11
1

@PerKristianGravdal Here `ssize_t ret = read(fd, &len, sizeof(len));` one integer value read from file and gets stored in `len`. if that read `len` value is greater than `1024`, then don't proceed further, return from function. – Achal Oct 29 '18 at 13:48

read() - Check buffer boundaries if used in a loop including recursive loops

1 Answers1

Linked