Ouch, that ancount increase seems quite weird. What if we still used
break to avoid printing all parts never added to message?
It seems to me multibyte variables in dns_header should be converted
just once after receiving and converted back to network order just once
before sending. Constant converting to network order and back seems
innecessary. Once we start working with dns_header structure, we may
convert it once to host format. Before we send it, convert it just once
to network order. That would be quite a change however. Can we convert
it just once in the function and once when finished? I think section
count increase should do directly add_resource_record function. It is
mandatory to do on every use, just variables used differ a bit. But
modification would require many places with just single line saved for
each call.
I mean this:
if (add_resource_record(header, limit, &trunc, sizeof(struct
dns_header), &p, daemon->local_ttl, NULL, T_A, C_IN, "4", &addr))
header->ancount++;
could become:
add_resource_record(header, limit, &trunc, &header->ancount,
sizeof(struct dns_header), &p, daemon->local_ttl, NULL, T_A, C_IN, "4",
&addr);
or similar to my change save few cycles when already known to be ignored:
if (!add_resource_record(header, limit, &trunc, &header->ancount,
sizeof(struct dns_header), &p, daemon->local_ttl, NULL, T_A, C_IN, "4",
&addr))
break;
I guess once truncation is indicated, there is no point continuing. Just
make sure TC flag is emitted, which might be in fact set also from
add_resource_record() itself. Header is there. Then send reply, no
matter what response records are already included.
Tought I expect this codepath is used rarely.
Regards,
Petr
On 10/6/21 00:47, Simon Kelley wrote:
> On 30/09/2021 23:49, Petr Menšík wrote:
>> Thanks!
>>
>> I were checking it a bit on test build and found part of file
>> 0013-Fix-coverity-issues-detected-in-domain-match.c.patch avoided
>> application. domain-match.c:447 still has add_resource_record return
>> value unchecked, unlike A record above.
>>
>> Btw, return values shadows truncp pointer. If add_resource_record ever
>> returns 0, truncp would always be set to 1. It seems trunc =
>> !add_resource_record() || trunc; Would give the same result without
>> passing it extra time as a pointer. Perhaps it should set { trunc = 1;
>> break; } on !add_resource_record() and not return 0 as I proposed first.
>>
> The intention of add_resource_record() (and the way it's used everywhere
> else) is that you can just keep calling it, and if any record goes over
> the packet limit, it will set *trunc, stop adding records, and return 0
> on subsequent calls. The only thing you have to do is not bump the
> section record counter if it returns zero, and set the trunc flag if
> *trunc is 1 by the end.
>
> This code fails to do the first, and therefore breaks.
>
>
> https://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=d2ad5dc073aaacaf22b117f16106282a73586803
>
> does it right.
>
>
> Cheers,
>
>
> Simon.
>
>> Attached alternative fix, which works better and were actually tested by:
>>
>> for F in {1..40}; do echo "--address=/test/127.0.0.$F"; done | xargs
>> src/dnsmasq -d --port 2053 --conf-file=/dev/null --log-queries
>>
>> for F in {1..20}; do echo "--address=/test/::$F"; done | xargs
>> src/dnsmasq -d --port 2053 --conf-file=/dev/null --log-queries
>>
>> Both create truncated responses to test a/aaaa queries. Current state
>> would not reply on A truncated query at all.
>>
>> On 9/12/21 00:05, Simon Kelley wrote:
>>> Applied in 2.87.
>>>
>>> Cheers,
>>>
>>> Simon.
>>>
>>>
>>>
>>> On 03/09/2021 22:47, Petr Menšík wrote:
>>>> Hi Simon,
>>>>
>>>> I have prepared a set of patches applied over 2.86rc3 release. They were
>>>> made to silent some of reports from Coverity scans we do for our
>>>> packages. I did include reported parts in commit messages, so commit
>>>> messages are somehow noisy and contain more bytes that the diffs itself.
>>>>
>>>> It should add few safety checks on multiple places. Fix few error paths
>>>> not releasing allocated memory and retries in case of failed syscall. It
>>>> is not perfect, but should be good enough.
>>>>
>>>> Not heavily tested, compiles without issues or warnings and reduced
>>>> reported issues. Review would be appreciated.
>>>>
>>>> What do you think, can they still be merged?
>>>>
>>>> Cheers,
>>>>
>>>> Petr
>>>>
>>>> On 8/25/21 3:46 PM, Simon Kelley wrote:
>>>>> I just pushed a few final changes, tagged as dnsmasq-2.86rc1.
>>>>>
>>>>> I'm fairly confident that this can be released as 2.86 in the near
>>>>> future, but if you can, please test it now, to avoid disappointment later.
>>>>>
>>>>> https://thekelleys.org.uk/dnsmasq/release-candidates/dnsmasq-2.86rc1.tar.gz
>>>>>
>>>>> Cheers,
>>>>>
>>>>> Simon.
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> 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
>>> _______________________________________________
>>> 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
>>
--
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemen...@redhat.com
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB
From 22799cc0b9091d5de638415bcd78cfa68b9d3a69 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com>
Date: Wed, 6 Oct 2021 17:24:43 +0200
Subject: [PATCH] Do no log every record in truncated message
They are not added to the reply sent back, avoid logging them if not
sent.
---
src/domain-match.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/domain-match.c b/src/domain-match.c
index 73f514e..7fab48b 100644
--- a/src/domain-match.c
+++ b/src/domain-match.c
@@ -401,7 +401,8 @@ size_t make_local_answer(int flags, int gotname, size_t size, struct dns_header
if (!(p = skip_questions(header, size)))
return 0;
-
+ header->ancount = ntohs(header->ancount);
+
if (flags & gotname & F_IPV4)
for (start = first; start != last; start++)
{
@@ -413,9 +414,10 @@ size_t make_local_answer(int flags, int gotname, size_t size, struct dns_header
else
continue;
- if (add_resource_record(header, limit, &trunc, sizeof(struct dns_header), &p, daemon->local_ttl, NULL, T_A, C_IN, "4", &addr))
- header->ancount = htons(ntohs(header->ancount) + 1);
- log_query((flags | F_CONFIG | F_FORWARD) & ~F_IPV6, name, (union all_addr *)&addr, NULL, 0);
+ if (!add_resource_record(header, limit, &trunc, sizeof(struct dns_header), &p, daemon->local_ttl, NULL, T_A, C_IN, "4", &addr))
+ break;
+ header->ancount++;
+ log_query((flags | F_CONFIG | F_FORWARD) & ~F_IPV6, name, &addr, NULL, 0);
}
if (flags & gotname & F_IPV6)
@@ -429,13 +431,15 @@ size_t make_local_answer(int flags, int gotname, size_t size, struct dns_header
else
continue;
- if (add_resource_record(header, limit, &trunc, sizeof(struct dns_header), &p, daemon->local_ttl, NULL, T_AAAA, C_IN, "6", &addr))
- header->ancount = htons(ntohs(header->ancount) + 1);
- log_query((flags | F_CONFIG | F_FORWARD) & ~F_IPV4, name, (union all_addr *)&addr, NULL, 0);
+ if (!add_resource_record(header, limit, &trunc, sizeof(struct dns_header), &p, daemon->local_ttl, NULL, T_AAAA, C_IN, "6", &addr))
+ break;
+ header->ancount++;
+ log_query((flags | F_CONFIG | F_FORWARD) & ~F_IPV4, name, &addr, NULL, 0);
}
if (trunc)
header->hb3 |= HB3_TC;
+ header->ancount = htons(header->ancount);
return p - (unsigned char *)header;
}
--
2.31.1
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
