[Dnsmasq-discuss] [PATCH] Re: 2.86rc1

Simon Kelley simon at thekelleys.org.uk
Wed Oct 6 21:51:19 UTC 2021


If you're going to use more natural internal representations of DNS
datastructures and then convert them to line format at the end, there
are much more deserving cases than this. I think I must have been
certifiably mad to do DNSSEC validation on a line-format representation
of a DNS packet. It ended up that way to stick to the design principle
that the normal code path for answering a DNS query should not need to
allocate heap memory.

The way it's done now seems to me to be fine. If you look at the
majority of the calls in rfc1035.c and auth.c then the section counter
is a variable in host byte order. There are a few places where other
things are done based on success or failure of the call. I've updated
make_local_answer() to do the same.

I don't think that removing RRs from the log because they are truncated
from the answer is a good idea. That just gets confusing. The truncated
answer is not going to be used anyway.

Cheers,

Simon.


On 06/10/2021 22:05, Petr Menšík wrote:
> 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 at lists.thekelleys.org.uk
>>>>>> https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Dnsmasq-discuss mailing list
>>>>>> Dnsmasq-discuss at lists.thekelleys.org.uk
>>>>>> https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
>>>> _______________________________________________
>>>> Dnsmasq-discuss mailing list
>>>> Dnsmasq-discuss at lists.thekelleys.org.uk
>>>> https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
>>>
>>> _______________________________________________
>>> Dnsmasq-discuss mailing list
>>> Dnsmasq-discuss at lists.thekelleys.org.uk
>>> https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
>>>




More information about the Dnsmasq-discuss mailing list