| Message ID | 20200415123138.17661-1-sbabic@denx.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] Fix regression by introducing strlcpy | expand |
Hi Stefano,
I was just about to submit a patch for the same regression (parsing `sw->software_set`), but you were a little bit
faster :-)
> The implemented strlcpy is buggy, fixed it.
Since I spent some time on this issue as well, I was wondering what is the origin of your `strlcpy` implementation? You
said [1] that you took it from FreeBSD, however the implementation there is different from yours and doesn't seem to
suffer from this bug:
- https://github.com/freebsd/freebsd/blob/master/sys/libkern/strlcpy.c
- https://github.com/freedesktop/libbsd/blob/master/src/strlcpy.c
Is it an issue with the BSD license?
Kind regards,
Michael
[1] https://groups.google.com/forum/#!topic/swupdate/wHPTpn4Tcc8
Hi Michael, On 15.04.20 15:26, Michael Adler wrote: > Hi Stefano, > > I was just about to submit a patch for the same regression (parsing `sw->software_set`), but you were a little bit > faster :-) I am running some tests before relase for end of the week, and I have hit these issues on a target. > >> The implemented strlcpy is buggy, fixed it. > > Since I spent some time on this issue as well, I was wondering what is the origin of your `strlcpy` implementation? You > said [1] that you took it from FreeBSD, The strlcpy() is present in FreeBSD and not in Linux, but solves the issue report by coverity because it ensures that a copied string is always terminated. After saying this, I have implemented myself (and adding a bug due to copy&paste from first lines, sigh !). > however the implementation there is different from yours and doesn't seem to > suffer from this bug: > > - https://github.com/freebsd/freebsd/blob/master/sys/libkern/strlcpy.c > - https://github.com/freedesktop/libbsd/blob/master/src/strlcpy.c I have seen these, but I thought that calling memcpy() (also because thare shoudl be an assembly version in libc) is faster as iterating the buffer. > > Is it an issue with the BSD license? No - I can take FreeBSD code and license it under GPLv2 as far as I know. Best regards, Stefano > > Kind regards, > Michael > > [1] https://groups.google.com/forum/#!topic/swupdate/wHPTpn4Tcc8 >
> After saying this, I have implemented myself (and adding a bug due to > copy&paste from first lines, sigh !). I see, so it's actually not the implementation from FreeBSD (FWIW, I think the origin of strlcpy is actually OpenBSD). > I have seen these, but I thought that calling memcpy() (also because > thare shoudl be an assembly version in libc) is faster as iterating the > buffer. `memcpy()` is certainly faster, but I wonder if it really matters for the usages in SWUpdate? However, having a secure and correct implementation of strlcpy certainly matters. For instance, I'm still a little wary about the usage of `strlen` in your implementation: calling `strlen` on a non-zero terminated string is undefined behavior. Yes, all passed in strings have to be zero-terminated. Fair enough, but people make mistakes and sooner or later someone will call this function with non-zero terminated input again. And in fact, it already happened in the case of `parse_image_selector`. So, maybe it's safer to use `strnlen` instead. But even then, I cannot say for sure that there are no other problems with it. Kind regards, Michael
On 15.04.20 16:13, Michael Adler wrote: >> After saying this, I have implemented myself (and adding a bug due to >> copy&paste from first lines, sigh !). > > I see, so it's actually not the implementation from FreeBSD (FWIW, I think the origin of strlcpy is actually OpenBSD). > >> I have seen these, but I thought that calling memcpy() (also because >> thare shoudl be an assembly version in libc) is faster as iterating the >> buffer. > > `memcpy()` is certainly faster, but I wonder if it really matters for the usages in SWUpdate? It does not really matter. > However, having a secure and correct implementation of strlcpy certainly matters. For instance, I'm still a little wary > about the usage of `strlen` in your implementation: calling `strlen` on a non-zero terminated string is undefined > behavior. Well, its is the base for a string copy.. > Yes, all passed in strings have to be zero-terminated. Fair enough, but people make mistakes and sooner or later someone > will call this function with non-zero terminated input again. And in fact, it already happened in the case of > `parse_image_selector`. So, maybe it's safer to use `strnlen` instead. Agree. > But even then, I cannot say for sure that there > are no other problems with it. So your suggestion is just to take FreeBSD implementation ? There are no license issues, so this can be done. Regards, Stefano > > Kind regards, > Michael >
> So your suggestion is just to take FreeBSD implementation ? There are no > license issues, so this can be done. Yep. That version is very well tested and has been reviewed by much smarter people than me. It's just too easy to get something wrong (or unsafe) in C. Thus, I would just go with upstream and worry about things in my software :) Kind regards, Michael
diff --git a/core/util.c b/core/util.c index 86057cf..0ed75ff 100644 --- a/core/util.c +++ b/core/util.c @@ -220,12 +220,17 @@ strlcpy(char *dst, const char * src, size_t size) { size_t len = strlen(src); + /* + * src is null termintaed, + * copy the last '\0', too. + */ if (len < size) { memcpy(dst, src, len + 1); } else if (len) { - memcpy(dst, src, len - 1); + /* truncate string */ + memcpy(dst, src, size - 1); /* Add C string terminator */ - dst[len - 1] = '\0'; + dst[size - 1] = '\0'; } return len; }
The implemented strlcpy is buggy, fixed it. Signed-off-by: Stefano Babic <sbabic@denx.de> --- core/util.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)