[Dnsmasq-discuss] v2.91test1 compile failure on FreeBSD

[Matthias Andree]

Am 24.12.24 um 12:07 schrieb Simon Kelley:
>
>
> On 22/12/2024 10:44, Matthias Andree via Dnsmasq-discuss wrote:
>> Am 20.12.24 um 22:16 schrieb Simon Kelley:
>>>
>>>
>>> On 12/20/24 12:27, Matthias Andree via Dnsmasq-discuss wrote:
>>>> Simon,
>>>>
>>>> I cannot compile v2.91test1 on FreeBSD 14.2, errors below. (Neither
>>>> tarball nor Git compile.) (2nd to last shown errors). Patch attached,
>>>> should be fine with git-am.
>>>
>>> Patch applied, thanks.
>>>>
>>>> Adding multiple unions with variable size into some other aggregate
>>>> (union or struct) isn't portable, first shown warning below, might be
>>>> an error on other or strictly set compilers. This needs fixing not
>>>> covered by my patch.
>>>
>>> If I've understood this right, then
>>>
>>> https://thekelleys.org.uk/gitweb/?
>>> p=dnsmasq.git;a=commit;h=4902807879c9b39db46c32e3e082a33266836d24
>>>
>>> should fix it.
>>
>> Thanks, but it's not sufficient, and my wording was unclear, sorry for
>> that.
>>
>> I am attaching an incremental git-am ready patch to go on top your
>> Git HEAD,
>> to fix all sorts of issues and make this conforming C99 with default
>> options set,
>> and fix another load of warnings you receive when setting the compiler
>> to pick the nits,
>> -pedantic-errors -std=c99 (or c11, c18, c2x).
>> It changes many void * to uint8_t * to make the "increment by bytes"
>> explicit.
>> You can't do:
>>
>> void *foo;
>> // ...
>> foo += 2.
>>
>
> Understood. There are few more instances of this in Linux-only code.
> I'll see to those too.
>
>> IMPORTANT: please review the char data[1] in my patch, whether you want
>> to increase the size.
>>
>> I see the six header warnings show below for practically all source
>> files compiled, and regarding the log message:
>> The warning is from FreeBSD 14.2's clang 18.1.6 compiler, not GCC, which
>> only shows them under -pedantic (I used GCC 13.3):
>>
>> The root cause for these warnings appears to be the VLA char data[]
>> inside union all_addr's struct datablock, see line 342 of src/dnsmasq.h:
>>
>>>    336    /* for arbitrary RR record small enough to go in addr.
>>>    337       NOTE: rrblock and rrdata are discriminated by the
>>> F_KEYTAG bit
>>>    338       in the cache flags. */
>>>    339    struct datablock {
>>>    340      unsigned short rrtype;
>>>    341      unsigned char datalen; /* also length of SOA in negative
>>> records. */
>>>    342      char data[];
>>>    343    } rrdata;
>>>    344  };
>>
>> Declaring this as char data[1] or data[16] or whatever is suitable
>> should fix the warnings. data[0] is non-conforming and won't work
>> either.
>> I don't know what's a good size here, as I haven't read enough context
>> code to judge that. My patch sets it 1 because that's the minimum in
>> order not to bloat the structure.
>
> data[1] will, I think, work fine.
>
> The plan here is to to have a union tag that stores the rrtype and a
> small length field and uses the rest of the space in the union to
> store RR data when that's small enough. The size of the union is at
> least 16 bytes, because it can store an IPv6 address, and I don't want
> it to get any larger than that.
>
> The line
>
> #define RR_IMDATALEN (sizeof(union all_addr) - offsetof(struct
> datablock, data))
>
> calculates how much data will fit and is used in the code. This is
> always 13 bytes at the moment. Sadly, the chicken-and-egg problem
> prevents the use of that expression in the declaration of data[]
>
> Declaring data[1] and then writing bytes up to data[12] clearly works
> whilst struct rrdata is part of a union of size 16, but I wonder if it
> will trip over array-overflow checking?
>
> Maybe we  should stop being clever,
>
> #define RR_IMDATALEN 13
>
> and declare data[RR_IMDATALEN];
>
> That at least stays away from language definition areas I don't claim
> to understand.

I think the previous RR_IMDATALEN expression is fine as long as you
#include <stddef.h> before its first use, and as along as you won't coin
your own version of offsetof, of which I don't know a
conforming/portable way.

The next thing you'll be missing is a _Static_assert to ensure
sizeof(data) <= 16.  Which isn't C99. :-)

Could you also push the v2.91test3 tag to the public Git? Its HEAD is at
7c348a0, there is also a test3 tarball, but no Git tag, and my patches
still apply on top of it, so it's older, my guess is 3ac11cd.

Thanks in adavance.