| Message ID | 20230927122744.3434851-3-thaller@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Two fixes to avoid "-Wstrict-overflow" warnings | expand |
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. BTW, did you test this patch with the pf.os file that nftables ships in? Thanks!
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
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.
On Wed, 2023-09-27 at 19:11 +0200, Pablo Neira Ayuso wrote: > On Wed, Sep 27, 2023 at 07:04:57PM +0200, Thomas Haller wrote: > > > > 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. Sorry, I don't follow. Testing this seems very cumbersome. I suspect, the tests "tests/shell/testcases/sets/typeof_{sets,maps}_0" might hit the code. But that test requires kernel support. IMO the netfilter projects should require contributors to provide tests (as sensible). That is, tests that are simply invoked via `make check` and don't require to build special features in the kernel (CONFIG_NFT_OSF). Anyway. Let's hold this patch [2/3] back for now. And patch [1/3] is obsolete too. I have patches that would add unit tests to the project (merely as a place where more unit tests could be added). I will add a test there. But that is based on top of "no recursive make", and I'd like to get that changed first. Thomas
On Wed, Sep 27, 2023 at 07:50:27PM +0200, Thomas Haller wrote: > On Wed, 2023-09-27 at 19:11 +0200, Pablo Neira Ayuso wrote: > > On Wed, Sep 27, 2023 at 07:04:57PM +0200, Thomas Haller wrote: > > > > > > 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. > > Sorry, I don't follow. Testing this seems very cumbersome. It requires kernel support and the pf.os file in place, yes. > I suspect, the tests "tests/shell/testcases/sets/typeof_{sets,maps}_0" > might hit the code. But that test requires kernel support. This requires kernel support, yes. > IMO the netfilter projects should require contributors to provide tests > (as sensible). That is, tests that are simply invoked via `make check` > and don't require to build special features in the kernel > (CONFIG_NFT_OSF). You mean, some way to exercise userspace code without involving the kernel at all. > Anyway. Let's hold this patch [2/3] back for now. And patch [1/3] is > obsolete too. OK, as you prefer. > I have patches that would add unit tests to the project (merely as a > place where more unit tests could be added). I will add a test there. We have tests/py/ as unit tests, if that might look similar to what you have in mind? Or are you thinking of more tests/shell/ scripts? > But that is based on top of "no recursive make", and I'd like to get > that changed first. I would like to make a release before such change is applied, build infrastructure and python support was messy in the previous release. Then we look into this, OK?
On Wed, 2023-09-27 at 21:16 +0200, Pablo Neira Ayuso wrote: > On Wed, Sep 27, 2023 at 07:50:27PM +0200, Thomas Haller wrote: > > > > IMO the netfilter projects should require contributors to provide > > tests > > (as sensible). That is, tests that are simply invoked via `make > > check` > > and don't require to build special features in the kernel > > (CONFIG_NFT_OSF). > > You mean, some way to exercise userspace code without involving the > kernel at all. Yes, the relevant part is parsing some strings. That should be tested in isolation. Or just to validate the pf.os file... > > > I have patches that would add unit tests to the project (merely as > > a > > place where more unit tests could be added). I will add a test > > there. > > We have tests/py/ as unit tests, if that might look similar to what > you have in mind? Or are you thinking of more tests/shell/ scripts? Those only use the public API of libnftables.so. What would be also useful, is to statically link against the code and have more immediate access. Also, currently they don't unshare and cannot run rootless. That should be fixed by extending tests/shell/run-tests.sh script. Well, you already hack that via `./tests/shell/run-tests.sh ./tests/py/nft- test.py`, but this should integrate better. It's waiting on the WIP branch: https://gitlab.freedesktop.org/thaller/nftables/-/commits/th/no-recursive-make https://gitlab.freedesktop.org/thaller/nftables/-/blob/545f40babb90584fd188ebe80a1103b93ba49707/tests/unit/test-libnftables-static.c#L177 > > > But that is based on top of "no recursive make", and I'd like to > > get > > that changed first. > > I would like to make a release before such change is applied, build > infrastructure and python support was messy in the previous release. > Then we look into this, OK? Sounds great. Thank you. Thomas
On Wed, Sep 27, 2023 at 10:11:15PM +0200, Thomas Haller wrote: > On Wed, 2023-09-27 at 21:16 +0200, Pablo Neira Ayuso wrote: > > On Wed, Sep 27, 2023 at 07:50:27PM +0200, Thomas Haller wrote: > > > > > > > IMO the netfilter projects should require contributors to provide > > > tests > > > (as sensible). That is, tests that are simply invoked via `make > > > check` > > > and don't require to build special features in the kernel > > > (CONFIG_NFT_OSF). > > > > You mean, some way to exercise userspace code without involving the > > kernel at all. > > Yes, the relevant part is parsing some strings. That should be tested > in isolation. Or just to validate the pf.os file... I understand, that would also require some sort of dump of the parsing, to validate this is correct. I think I understand what you mean by unit test here: You could make a program that imports this .c file, the parse pf.os and dump an output that you could validate in some automated fashion. This osf support from iptables, and tests/py (which was the automated test infrastructure it had) was only added 2012, more than 10 years after iptables was in place. > > > I have patches that would add unit tests to the project (merely as > > > a > > > place where more unit tests could be added). I will add a test > > > there. > > > > We have tests/py/ as unit tests, if that might look similar to what > > you have in mind? Or are you thinking of more tests/shell/ scripts? > > Those only use the public API of libnftables.so. What would be also > useful, is to statically link against the code and have more immediate > access. I see, some internal tests for private API then it is your idea, I am all in for more test coverage. > Also, currently they don't unshare and cannot run rootless. That should > be fixed by extending tests/shell/run-tests.sh script. Well, you > already hack that via `./tests/shell/run-tests.sh ./tests/py/nft- > test.py`, but this should integrate better. Yes, unshare and rootless for tests/py would be good to have if I understood this correctly. > It's waiting on the WIP branch: > https://gitlab.freedesktop.org/thaller/nftables/-/commits/th/no-recursive-make > https://gitlab.freedesktop.org/thaller/nftables/-/blob/545f40babb90584fd188ebe80a1103b93ba49707/tests/unit/test-libnftables-static.c#L177 > > > > > > > But that is based on top of "no recursive make", and I'd like to > > > get > > > that changed first. > > > > I would like to make a release before such change is applied, build > > infrastructure and python support was messy in the previous release. > > Then we look into this, OK? > > Sounds great. Thank you. OK, let's prepare for release then.
diff --git a/src/nfnl_osf.c b/src/nfnl_osf.c index 38a27a3683e2..f2b50fa9fc8e 100644 --- a/src/nfnl_osf.c +++ b/src/nfnl_osf.c @@ -74,6 +74,33 @@ static struct nf_osf_opt IANA_opts[] = { { .kind=26, .length=1,}, }; +static char *skip_delim(char *ptr, char c) +{ + char *tmp; + + tmp = strchr(ptr, c); + if (tmp) { + tmp++; + while (isspace(tmp[0])) + tmp++; + } + return tmp; +} + +static char *skip_delim_trunc(char *ptr, char c) +{ + char *tmp; + + tmp = strchr(ptr, c); + if (tmp) { + tmp[0] = '\0'; + tmp++; + while (isspace(tmp[0])) + tmp++; + } + return tmp; +} + static char *nf_osf_strchr(char *ptr, char c) { char *tmp; @@ -88,54 +115,34 @@ static char *nf_osf_strchr(char *ptr, char c) return tmp; } -static void nf_osf_parse_opt(struct nf_osf_opt *opt, __u16 *optnum, char *obuf, - int olen) +static void nf_osf_parse_opt(struct nf_osf_opt *opt, __u16 optlen, __u16 *optnum, char *obuf) { - int i, op; - char *ptr, wc; - unsigned long val; - - ptr = &obuf[0]; - i = 0; - while (ptr != NULL && i < olen && *ptr != 0) { - val = 0; - wc = OSF_WSS_PLAIN; - switch (obuf[i]) { + char *ptr = &obuf[0]; + + while (ptr && ptr[0] != '\0') { + char *const ptr0 = ptr; + unsigned long val = 0; + char wc = OSF_WSS_PLAIN; + int op; + + switch (ptr0[0]) { case 'N': op = OSFOPT_NOP; - ptr = nf_osf_strchr(&obuf[i], OPTDEL); - if (ptr) { - *ptr = '\0'; - ptr++; - i += (int)(ptr - &obuf[i]); - } else - i++; + ptr = skip_delim(&ptr0[1], OPTDEL); break; case 'S': op = OSFOPT_SACKP; - ptr = nf_osf_strchr(&obuf[i], OPTDEL); - if (ptr) { - *ptr = '\0'; - ptr++; - i += (int)(ptr - &obuf[i]); - } else - i++; + ptr = skip_delim(&ptr0[1], OPTDEL); break; case 'T': op = OSFOPT_TS; - ptr = nf_osf_strchr(&obuf[i], OPTDEL); - if (ptr) { - *ptr = '\0'; - ptr++; - i += (int)(ptr - &obuf[i]); - } else - i++; + ptr = skip_delim(&ptr0[1], OPTDEL); break; case 'W': op = OSFOPT_WSO; - ptr = nf_osf_strchr(&obuf[i], OPTDEL); + ptr = skip_delim_trunc(&ptr0[1], OPTDEL); if (ptr) { - switch (obuf[i + 1]) { + switch (ptr0[1]) { case '%': wc = OSF_WSS_MODULO; break; @@ -149,56 +156,37 @@ static void nf_osf_parse_opt(struct nf_osf_opt *opt, __u16 *optnum, char *obuf, wc = OSF_WSS_PLAIN; break; } - - *ptr = '\0'; - ptr++; - if (wc) - val = strtoul(&obuf[i + 2], NULL, 10); + if (wc != OSF_WSS_PLAIN) + val = strtoul(&ptr0[2], NULL, 10); else - val = strtoul(&obuf[i + 1], NULL, 10); - i += (int)(ptr - &obuf[i]); - - } else - i++; + val = strtoul(&ptr0[1], NULL, 10); + } break; case 'M': op = OSFOPT_MSS; - ptr = nf_osf_strchr(&obuf[i], OPTDEL); + ptr = skip_delim_trunc(&ptr0[1], OPTDEL); if (ptr) { - if (obuf[i + 1] == '%') + if (ptr0[1] == '%') wc = OSF_WSS_MODULO; - *ptr = '\0'; - ptr++; - if (wc) - val = strtoul(&obuf[i + 2], NULL, 10); + if (wc != OSF_WSS_PLAIN) + val = strtoul(&ptr0[2], NULL, 10); else - val = strtoul(&obuf[i + 1], NULL, 10); - i += (int)(ptr - &obuf[i]); - } else - i++; + val = strtoul(&ptr0[1], NULL, 10); + } break; case 'E': op = OSFOPT_EOL; - ptr = nf_osf_strchr(&obuf[i], OPTDEL); - if (ptr) { - *ptr = '\0'; - ptr++; - i += (int)(ptr - &obuf[i]); - } else - i++; + ptr = skip_delim(&ptr0[1], OPTDEL); break; default: op = OSFOPT_EMPTY; - ptr = nf_osf_strchr(&obuf[i], OPTDEL); - if (ptr) { - ptr++; - i += (int)(ptr - &obuf[i]); - } else - i++; + ptr = skip_delim(&ptr0[0], OPTDEL); break; } if (op != OSFOPT_EMPTY) { + if (*optnum >= optlen) + return; opt[*optnum].kind = IANA_opts[op].kind; opt[*optnum].length = IANA_opts[op].length; opt[*optnum].wc.wc = wc; @@ -235,7 +223,7 @@ static int osf_load_line(char *buffer, int len, int del, return -EINVAL; } - memset(obuf, 0, sizeof(obuf)); + obuf[0] = '\0'; pbeg = buffer; pend = nf_osf_strchr(pbeg, OSFPDEL); @@ -320,7 +308,7 @@ static int osf_load_line(char *buffer, int len, int del, pbeg = pend + 1; } - nf_osf_parse_opt(f.opt, &f.opt_num, obuf, sizeof(obuf)); + nf_osf_parse_opt(f.opt, NFT_ARRAY_SIZE(f.opt), &f.opt_num, obuf); memset(buf, 0, sizeof(buf));
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. Signed-off-by: Thomas Haller <thaller@redhat.com> --- src/nfnl_osf.c | 128 ++++++++++++++++++++++--------------------------- 1 file changed, 58 insertions(+), 70 deletions(-)