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.
Also note that many of the *_addr types you define are
memory-inefficient because you often have short types before pointer
types, which have alignment requirements almost everywhere, and
something like:
struct {
unsigned short rrtype;
unsigned short datalen;
struct blockdata *rrdata;
} rrblock;
will incur 4 bytes of padding between datalen and rrdata so the pointer
is aligned at 64-bit boundaries. The standing recommendation is to sort
members by descending order of alignment requirement and size. I. e.
pointers and longs first, then ints, then shorts.
For this particular example, you could write:
struct {
struct blockdata *rrdata;
unsigned short rrtype;
unsigned short datalen;
} rrblock;
The reason for doing this is that it allows code to find the value of
rrtype from a strut rrblock OR struct rrdata all_addr without doing
different code path for each. No space is wasted in rrblock since the
all_addr union is 16 bytes to store an IPv6 address, and the struct as
defined fits that. It's important to have the rrtype at the start of
struct rrdata to maximise the size of the data field.
to fix that. IMPORTANT: my patch does not address this!
In many cases, size-definite types (such as uint16_t) are a good choice
if you really know the value range, say, from IETF standards and RFCs,
but would require lots of printf-style format adjustments, too.
./dnsmasq.h:350:18: warning: field 'addr' with variable sized type
'union all_addr' not at the end of a struct or class is a GNU
extension [-Wgnu-variable-sized-type-not-at-end]
350 | union all_addr addr;
| ^
./dnsmasq.h:416:18: warning: field 'addr' with variable sized type
'union all_addr' not at the end of a struct or class is a GNU
extension [-Wgnu-variable-sized-type-not-at-end]
416 | union all_addr addr;
| ^
./dnsmasq.h:483:18: warning: field 'addr' with variable sized type
'union all_addr' not at the end of a struct or class is a GNU
extension [-Wgnu-variable-sized-type-not-at-end]
483 | union all_addr addr;
| ^
./dnsmasq.h:782:20: warning: field 'dest' with variable sized type
'union all_addr' not at the end of a struct or class is a GNU
extension [-Wgnu-variable-sized-type-not-at-end]
782 | union all_addr dest;
| ^
./dnsmasq.h:787:5: warning: field 'frec_src' with variable sized type
'struct frec_src' not at the end of a struct or class is a GNU
extension [-Wgnu-variable-sized-type-not-at-end]
787 | } frec_src;
| ^
./dnsmasq.h:1108:18: warning: field 'source' with variable sized type
'union all_addr' not at the end of a struct or class is a GNU
extension [-Wgnu-variable-sized-type-not-at-end]
1108 | union all_addr source;
| ^
GCC shows such warnings in stricter modes (for instance, -pedantic),
clang already does with default options.
There is also this char * vs. void * complaint:
bpf.c: In function 'arp_enumerate':
bpf.c:94:40: warning: passing argument 2 of 'callback.af_unspec' from
incompatible pointer type [-Wincompatible-pointer-types]
94 | if (!callback.af_unspec(AF_INET, &sin2->sin_addr,
LLADDR(sdl), sdl->sdl_alen, parm))
| ^~~~~~~~~~~~~~~
| |
| struct in_addr *
bpf.c:94:40: note: expected 'char *' but argument is of type 'struct
in_addr *'
A simple fix for this one is changing the inf (*af_unspec)...
declaration to make void *addrp instead of char *addrp.
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
