remove strcpy, and replace with an iio_strlcpy #439
Conversation
rgetz
force-pushed
the
rgetz-remove-strcpy
branch
2 times, most recently
from
edf6ce1 to
d705f3e
Compare
|
Minor tweaks for Windows, and flawfinder. This does fix 18 issues.
|
|
This is part of #441 |
rgetz
force-pushed
the
rgetz-remove-strcpy
branch
from
3930019 to
67b61fe
Compare
|
The issue that I was trying to avoid is allowing the dest not to be null terminated for some reason.
strncpy_s on Windows has a different function prototype (needs the length to copy, and the length of the dest). But now that its not the weekend - I get having a standard library function that does not operate like the standard library function. I can just borrow the actual BSD function, and use that: https://github.com/freebsd/freebsd/blob/master/sys/libkern/strlcpy.c Right now, In the calling functions, there is no checking to see if things were OK, it just decrements length, and moves on, this is OK as long as length doesn't copy anything when it's negative, but this doesn't comply with the std function - which is how I got here... Can do either. Suggestions? |
We used strcpy a few places, which opens up classic buffer overflow issues so check those, and fix them. https://cwe.mitre.org/data/definitions/120.html This adds some internal error checking, as we are building up xml representations of things, we now keep track of the remaining space in the buffer, and don't go over if we have no space. (which in theory should never happen). Tested on Pluto and M2k to see if this introduces issues, and couldn't find anything. Signed-off-by: Robin Getz <[email protected]>
According to https://cwe.mitre.org/data/definitions/134.html describes functions that accepts a format string as an argument, but the format string originates from an external source as dangerous. This gets tagged on all the IIO_DEBUG, IIO_INFO, IIO_WARNING, and IIO_ERRRO functions that are in debug.h However, they are always called internally from the library, have fixed format strings and can not be modified externally, so mark them as safe. Signed-off-by: Robin Getz <[email protected]>
https://cwe.mitre.org/data/definitions/120.html points out that sprintf will overflow buffers, so don't use it. We move to the internal iio_snprintf, and move a few remaining snprintf to the same to handle the necessary windows/Linux differences. Signed-off-by: Robin Getz <[email protected]>
strncpy is easily used incorrectly since it doesn't always \0-terminate or check for invalid pointers. However, iio_strlcpy does, so use that. this is part of CWE-120 https://cwe.mitre.org/data/definitions/120.html Signed-off-by: Robin Getz <[email protected]>
rgetz
force-pushed
the
rgetz-remove-strcpy
branch
from
ba95495 to
5bc984d
Compare
|
This uses a standard strlcpy implementation from BSD. |
| size_t *attrs_len, scan_element_len = 0; | ||
| unsigned int i; | ||
|
|
||
| len = sizeof("<channel id=\"\" type=\"\" ></channel>"); |
There was a problem hiding this comment.
for important files like channel.c this patch is doing too much in one go;
i can understand the value of converting
size_t len = sizeof("<channel id=\"\" name=\"\" "
"type=\"output\" ></channel>")
+ strlen(chn->id) + (chn->name ? strlen(chn->name) : 0);
to
len = sizeof("<channel id=\"\" type=\"\" ></channel>");
len += strnlen(chn->id, MAX_CHN_ID);
len += chn->is_output ? sizeof("output") : sizeof("input");
len += chn->name ? sizeof(" name= ") + strnlen(chn->name, MAX_CHN_NAME) : 0;
but it should be in it's own patch;
otherwise we risk slipping stuff at review;
There was a problem hiding this comment.
sure - I can split up this commit into multiple.
|
Why not just use |
that is basically what strlcpy does. Making a helper function that does that, is close to the same - isn't it? |
|
I'm going to close this (unmerged) so I can make a new one with a split out version per @commodo 's request. |
|
can the branch associated with this PR be removed? |
We used strcpy a few places, which opens up classic buffer overflow
issues so check those, and fix them.
https://cwe.mitre.org/data/definitions/120.html
This adds some internal error checking, as we are building up xml
representations of things, we now keep track of the remaining space
in the buffer, and don't go over if we have no space. (which in theory
should never happen).
Tested on Pluto and M2k to see if this introduces issues, and couldn't
find anything.
Signed-off-by: Robin Getz [email protected]