Potential buffer overflow

[megakilo]

In the endpoints.c file, there are a lot of strncat calls.
For example: strncat(rsp, ">;", len);
The third parameter should be the max length to be appended from the string at the second parameter. So it should not be "len".
Also to avoid buffer overflow, I think it should be:
strncat(rsp, ">;", len-strlen(rsp)); //len is already the buffer size without \0

Is my understanding right?

[tekjar]

+1

strncat(buff, "String 2", BUFFER_SIZE - strlen(buff) - 1);

[pcsrule]

All of the strncat calls in endpoints.c look fine to me. Subtracting 1 from len at the beginning of endpoint_setup leaves room for any nul char, and len is used as a maximum in all subsequent strncat calls to prevent overflow. Each time strncat is called, the appropriate number of bytes is subtracted from len to make sure it still represents the number of characters left in the buffer minus the nul char.

An argument could be made for a more appropriate variable name than len however, as I think that is the source of the confusion. Perhaps buf_remaining?

[megakilo]

@kteza1 your code is generally right, but for microcoap, it already subtracts 1 at the beginning.

@pcsrule agree that the null terminator space is reserved already. But the "len" parameter of strncat (i.e. the 3rd argument) is the length of the source (2nd argument) not the destination (1st argument). So when the strncat concatenate strings, it doesn't know how much space left in the destination, thus could cause overflow.
http://linux.die.net/man/3/strncat

[pcsrule]

The len variable is the length available to write to in the destination, independent of the source. It is not the length of the source. The implementation is correct as is. len is set to rsplen at the beginning of endpoint_setup which is 1500, the length of the rsp buffer being written to. len is decremented as rsp is written to in order to reduce the maximum number of characters that can be written. It is not necessarily the number of characters that will be written, only a maximum to prevent overflow. strncat will only copy up to the nul char in the source, or when the number of chars indicated by the third parameter have been written, whichever comes first.

[megakilo]

@pcsrule the 3rd argument of strncat is the length of the source not destination. strncat has no idea about the dest length. I think you get confused with snprintf. You can do "man strncat" on your machine to look at the definition and its implementation.

[tekjar]

@pcsrule @megakilo Sorry I commented without seeing the full code properly. @pcsrule is correct. len in this implementation is current num of bytes available in rsp. For every strncat in this code, he is decrementing the available size (which is len) in rsp

@megakilo

strncat has no idea about the dest length.

strncat has idea about neither src not dest lenght. All it cares is not concat more than 3rd arg to dest

This issue can be closed IMO