On Wed, 2023-09-27 at 18:42 +0200, Pablo Neira Ayuso wrote: > On Wed, Sep 27, 2023 at 02:23:27PM +0200, Thomas Haller wrote: > > We almost can compile everything with "-Wstrict-overflow" (which > > depends > > on the optimization level). In a quest to make that happen, rework > > nf_osf_parse_opt(). Previously, gcc-13.2.1-1.fc38.x86_64 warned: > > > > $ gcc -Iinclude "-DDEFAULT_INCLUDE_PATH=\"/usr/local/etc\"" -c > > -o tmp.o src/nfnl_osf.c -Werror -Wstrict-overflow=5 -O3 > > src/nfnl_osf.c: In function ‘nfnl_osf_load_fingerprints’: > > src/nfnl_osf.c:356:5: error: assuming signed overflow does not > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [- > > Werror=strict-overflow] > > 356 | int nfnl_osf_load_fingerprints(struct netlink_ctx *ctx, > > int del) > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > src/nfnl_osf.c:356:5: error: assuming signed overflow does not > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [- > > Werror=strict-overflow] > > src/nfnl_osf.c:356:5: error: assuming signed overflow does not > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [- > > Werror=strict-overflow] > > src/nfnl_osf.c:356:5: error: assuming signed overflow does not > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [- > > Werror=strict-overflow] > > src/nfnl_osf.c:356:5: error: assuming signed overflow does not > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [- > > Werror=strict-overflow] > > src/nfnl_osf.c:356:5: error: assuming signed overflow does not > > occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [- > > Werror=strict-overflow] > > cc1: all warnings being treated as errors > > > > The previous code was needlessly confusing. Keeping track of an > > index > > variable "i" and a "ptr" was redundant. The signed "i" variable > > caused a > > "-Wstrict-overflow" warning, but it can be dropped completely. > > > > While at it, there is also almost no need to ever truncate the bits > > that > > we parse. Only the callers of the new skip_delim_trunc() required > > the > > truncation. > > > > Also, introduce new skip_delim() and skip_delim_trunc() methods, > > which > > point right *after* the delimiter to the next word. Contrary to > > nf_osf_strchr(), which leaves the pointer at the end of the > > previous > > part. > > > > Also, the parsing code using strchr() requires that the overall > > buffer > > (obuf[olen]) is NUL terminated. And the caller in fact ensured that > > too. > > There is no point in having a "olen" parameter, we require the > > string to > > be NUL terminated (which already was implicitly required). Drop > > the > > "olen" parameter. On the other hand, it's unclear what ensures that > > we > > don't overflow the "opt" output buffer. Pass a "optlen" parameter > > and > > ensure we don't overflow the buffer. > > Nice. > > IIRC, this code was copied and pasted from iptables. Maybe porting > this patch there would be also good. I will do that, after the patch was merged (and the final version known). > BTW, did you test this patch with the pf.os file that nftables ships > in? Right. I need to point out, that I did not test this. So it might be horribly broken. My Fedora kernel builds without CONFIG_NFT_OSF, so the shell tests are skipped. How can pf.os used? Thomas
