Issues with socket buffer reader

Question

I've been given this code for an assignment, there supposed to be errors in it but I can't actually figure out what this function is supposed to do, never mind figure out if there's any issues with it...

I am guessing that it's supposed to read the buffer line by line, but I've never seen it done this way before

The buffer that is sent to the function is empty.

int read_line(int sock, char *buffer) {
    size_t length = 0;

    while (1) {
        char data;
        int result = recv(sock, &data, 1, 0);

        if ((result <= 0) || (data == EOF)){
            perror("Connection closed");
            exit(1);
        }

        buffer[length] = data;
        length++;

        if (length >= 2 && buffer[length-2] == '\r' && buffer[length-1] == '\n') {
            buffer[length-2] = '\0';
            return length;
        }
    }
}

Thanks in advance!

Perhaps the type of return value is wrong, and so is the actual length - why'd you return the length of line with the line terminator when the line terminator has been stripped out... — Antti Haapala -- Слава Україні, May 28 '19 at 11:57
However the **greatest error of all is the [one design error shared with `gets`](https://stackoverflow.com/questions/1694036/why-is-the-gets-function-so-dangerous-that-it-should-not-be-used)!** — Antti Haapala -- Слава Україні, May 28 '19 at 11:57

score 2 · Accepted Answer · answered May 28 '19 at 15:10

I'd say the purpose of this function is to read a line that ends with \r\n from socket stream and store it in a char array as a string, therefore the \0 (string termination character) placement.

Ok, so what's wrong with the code?

I'd start with the input parameter char *buffer - inside the function you do not know its size so you cannot check if it exceeds its size limit and it could lead to buffer overflow. So it would be better to send buffer length as a parameter and check with every received byte if it can be stored.

EOF - it is defined as -1 and in this case actually doesn't make any sense, because nothing will be setting your data variable to EOF. The only thing you need to look out for is the end of socket stream (recv documentation). And here is an example for EOFusage. Feel free to remove (data == EOF) from condition.

Let's say you are receiving everything regularly and you receive your last input and connection closes, so you enter this case:

if ((result <= 0) || (data == EOF)){
    perror("Connection closed");
    exit(1);
}

The problem here is that you won't process your last line and the program will just end. Although, I might be wrong here since I don't know when the connection is getting regularly shut down. And a minor note here, result that equals to 0 isn't considered as an error, but a regular connection shutdown (or a 0-byte datagram was received).

I hope I haven't missed anything.

Issues with socket buffer reader

1 Answers1