| Message ID | 20210831082553.430839-1-vincent@systemli.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Revert "initd: fix off-by-one error in mkdev.c" | expand |
On 2021-08-31 10:25, vincent@systemli.org wrote: > From: Nick Hainke <vincent@systemli.org> > > This reverts commit 8eb1d783cca6e0d501dd3a2f94262ffc36ae6482. > > This line reads a symbolic link into the string buffer "buf". > len = readlink(buf2, buf, sizeof(buf)); > The commit replaced now > buf[len] = 0; > with > buf[sizeof(buf) - 1] = '\0'; > > However, that does not work since readlink does not null-terminate > the string written into "buf" and "buf[len] = 0" was used for that. > > What happens if the buffer is to small? > "If the buf argument is not large enough to contain the link content, > the first bufsize bytes shall be placed in buf." > (Source: https://pubs.opengroup.org/onlinepubs/009695399/functions/readlink.htm) That revert adds back the original off-by-one error, since len will be sizeof(buf) in case of an undersized buffer. I agree that 'buf[len] = 0' is correct, but only if you also use sizeof(buf)-1 as size argument in the readlink() call. - Felix
Yep. Thanks. Just added another patch that is fixing the issue. I went to some internet sources and code to see how other people handle the issue and seems like everyone is just subtracting 1. Now you also wrote the same. :) Bests, Nick On 8/31/21 10:59 AM, Felix Fietkau wrote: > On 2021-08-31 10:25, vincent@systemli.org wrote: >> From: Nick Hainke <vincent@systemli.org> >> >> This reverts commit 8eb1d783cca6e0d501dd3a2f94262ffc36ae6482. >> >> This line reads a symbolic link into the string buffer "buf". >> len = readlink(buf2, buf, sizeof(buf)); >> The commit replaced now >> buf[len] = 0; >> with >> buf[sizeof(buf) - 1] = '\0'; >> >> However, that does not work since readlink does not null-terminate >> the string written into "buf" and "buf[len] = 0" was used for that. >> >> What happens if the buffer is to small? >> "If the buf argument is not large enough to contain the link content, >> the first bufsize bytes shall be placed in buf." >> (Source: https://pubs.opengroup.org/onlinepubs/009695399/functions/readlink.htm) > That revert adds back the original off-by-one error, since len will be > sizeof(buf) in case of an undersized buffer. > I agree that 'buf[len] = 0' is correct, but only if you also use > sizeof(buf)-1 as size argument in the readlink() call. > > - Felix > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On Tue, Aug 31, 2021 at 11:13:31AM +0200, Nick wrote: > Yep. Thanks. Just added another patch that is fixing the issue. I went to > some internet sources and code to see how other people handle the issue and > seems like everyone is just subtracting 1. Now you also wrote the same. :) Yes, that's correct fix, I have applied it to procd.git and will update the package in openwrt.git in a moment. Sorry for the mess everybody. Cheers Daniel > > Bests, > Nick > > On 8/31/21 10:59 AM, Felix Fietkau wrote: > > On 2021-08-31 10:25, vincent@systemli.org wrote: > > > From: Nick Hainke <vincent@systemli.org> > > > > > > This reverts commit 8eb1d783cca6e0d501dd3a2f94262ffc36ae6482. > > > > > > This line reads a symbolic link into the string buffer "buf". > > > len = readlink(buf2, buf, sizeof(buf)); > > > The commit replaced now > > > buf[len] = 0; > > > with > > > buf[sizeof(buf) - 1] = '\0'; > > > > > > However, that does not work since readlink does not null-terminate > > > the string written into "buf" and "buf[len] = 0" was used for that. > > > > > > What happens if the buffer is to small? > > > "If the buf argument is not large enough to contain the link content, > > > the first bufsize bytes shall be placed in buf." > > > (Source: https://pubs.opengroup.org/onlinepubs/009695399/functions/readlink.htm) > > That revert adds back the original off-by-one error, since len will be > > sizeof(buf) in case of an undersized buffer. > > I agree that 'buf[len] = 0' is correct, but only if you also use > > sizeof(buf)-1 as size argument in the readlink() call. > > > > - Felix > > > > _______________________________________________ > > openwrt-devel mailing list > > openwrt-devel@lists.openwrt.org > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff --git a/initd/mkdev.c b/initd/mkdev.c index 1c9c97a..44101aa 100644 --- a/initd/mkdev.c +++ b/initd/mkdev.c @@ -86,7 +86,7 @@ static void find_devs(bool block) if (len <= 0) continue; - buf[sizeof(buf) - 1] = '\0'; + buf[len] = 0; if (!find_pattern(buf)) continue;