[Dnsmasq-discuss] Dangerous compiler warning "fix" in 942a35f (was: dnsmasq 2.92test2: Compiler warnings)

[Matthias Andree]

Am 08.04.25 um 11:37 schrieb Opty:
> Hello,
>
> could you silence the following compiler warnings?
>
> cache.c: In function ‘cache_recv_insert’:
> cache.c:884:17: warning: comparison of integer expressions of
> different signedness: ‘ssize_t’ {aka ‘int’} and ‘u32’ {aka ‘unsigned
> int’} [-Wsign-compare]
>    884 |           if (m > daemon->metrics[METRIC_CRYPTO_HWM])
>        |                 ^
> cache.c:888:17: warning: comparison of integer expressions of
> different signedness: ‘ssize_t’ {aka ‘int’} and ‘u32’ {aka ‘unsigned
> int’} [-Wsign-compare]
>    888 |           if (m > daemon->metrics[METRIC_SIG_FAIL_HWM])
>        |                 ^
> cache.c:892:17: warning: comparison of integer expressions of
> different signedness: ‘ssize_t’ {aka ‘int’} and ‘u32’ {aka ‘unsigned
> int’} [-Wsign-compare]
>    892 |           if (m > daemon->metrics[METRIC_WORK_HWM])
>        |                 ^
>
> dnssec.c: In function ‘dnssec_validate_ds’:
> dnssec.c:995:22: warning: ‘rc’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
>    995 |   int qtype, qclass, rc, i, neganswer = 0, nons = 0, servfail
> = 0, neg_ttl = 0, found_supported = 0;
>        |                      ^~
>
Hello,

this apparently prompted a change which I deem dangerous.

TL;DR: WARNING: this behaves differently on different systems depending 
on natural word width, and the committed patch looks suspicious to me 
and does not resolve the ambiguity of what happens under the bonnet, it 
only hides it from the compiler.
I am writing about this post 2.92test2 commit:

> commit 942a35f5177746d2080e7aa118dd1493a500e2d5 (origin/master, 
> origin/HEAD)
> Author: Opty <opty77 at gmail.com>
> Date:   Wed Apr 16 16:00:47 2025 +0100
>
>    Silence compiler warnings.
>
>
1. Why would it be safe to read() or write() a "packet" directly from/to 
a ssize_t through read_write()? Who, furthermore, guarantees that the 
counterpart uses proper endianness, width, and signedness?
I may be missing context, but using a ssize_t as buffer for opaque 
memory seems wrong. If it's a byte string, why not use a char 
m[some_reasonable_width] here instead of a ssize_t that dnsmasq does not 
control?

2. There is an ambiguity here depending on the relation of 
sizeof(ssize_t) == ? (system dependent) versus sizeof(u32) == 4.

2a. On systems where ssize_t is "int" (which itself is suspicious, it 
should be long or long long, at any rate, usually 64 bits wide unless 
it's a really low-end or embedded system still living in the 32-bit world),
the comparison will most likely take place in the (unsigned) uint32_t 
domain and cast m to unsigned where it may wrap and become UINT32_MAX or 
worse if m == -1.
If that uses integer wraparound of unsigned as desired side effect, it 
should be documented, best in proper code, at least in a comment.

2b. On systems where ssize_t instead is long or long long and 64-bits 
wide (or more generally, wider than int), the type outranks the u32, and
instead the comparison will take place in the (signed) int64_t domain 
and will *not* overflow or wrap *any* value from the right-hand side.

Simon, either convine me that 942a35 does the right thing (and add 
comments) or please revert and add or seek assistance for a proper fix.

TIA.

Regards,
Matthias
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20250417/3a0922c7/attachment-0001.htm>