On Wed, Sep 27, 2023 at 07:04:57PM +0200, Thomas Haller wrote: > 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? According to code, pf.os file with signatures needs to be placed here: #define OS_SIGNATURES DEFAULT_INCLUDE_PATH "/nftables/osf/pf.os" then, you can start matching on OS type, see 'osf' expression in manpage. Note there is a "unknown" OS type when it does not guess the OS.
