[Dnsmasq-discuss] Dangerous compiler warning "fix" in 942a35f

[Simon Kelley]

On 4/17/25 07:52, Matthias Andree wrote:
> 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?

The "packet" in this case is being read from a unix pipe and is sent by 
a fork()ed instance of the same process. Several things are therefore 
undoubtedly true: 1) The sender and receiver both agree on the 
endianness and  width, 2) the type is signed (that's what the initial s 
in ssize_t conventionally means).

The value of m here is partially used as a union discriminator: if m is 
positive, the process is receiving a cache entry, and the value of m is 
the size of the single variable-sized field in the structure: the name.
If it's negative, then what is being received is one of two other 
structures, the code in question runs when m is -1 which indicates the 
end of a cache insertion and it gets gets updated stats. If m is -2, 
then it's receiving the result of a DNSSEC validation which has been 
moved from the UDP codepath to the TCP codepath.

So the type has to be capable of representing negative numbers, and, in 
general, the length of an arbitrary string. In fact, the string is 
limited to the maximum size of the DNS name, so a 32 bit or even 16 bit 
value would suffice, but it's passed as the length parameter to 
read_write(), so making it something else would involve a cast there.


> 
> 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.


I can't argue with any of that, but in practice, the value of any of the 
stats high-water-mark counters is not going to exceed 2^31, so the point 
is moot.

Tidying this up is pretty easy, just fix cache_end_insert(), above, and 
this code, to pass unsigned ints down the pipe. I think the only reason 
is wasn't done like that was to avoid declaring another variable, which 
is not a good reason. I'll make the change, it's a good thing to do.

Cheers,

Simon.

> 
> Regards,
> Matthias
>