[Dnsmasq-discuss] [BUG] [PATCH] Segmentation fault in src/forward.c

Petr Menšík pemensik at redhat.com
Fri Sep 24 22:47:56 UTC 2021


I am afraid I forgot to actually send the first bigger part, explaining
my motivation.

Replaced patches in original message with the latest one. More comments
below.

On 9/24/21 00:36, Simon Kelley wrote:
> On 23/09/2021 18:16, Petr Menšík wrote:
>> I am quite sure also --rebind-domain-ok is broken. It allocates struct
>> serv_local, but does not set any flags if I saw it correctly. Therefore
>> it would be later threated as struct server and checked in wrong places.
> Those structures are kept on a completely separate linked-list which is
> only touched by  domain_no_rebind() which doesn't use the flags field at
> all, so the code is correct, if possibly confusing. One could define a
> different structure to eliminate the redundant flags field and make
> things clearer. A bigger problem is possibly that this doesn't call the
> IDN infrastructure.

It is not the only part. I found just by coincidence,
--local=/münich.de/ does not work either. Maybe domain names should be
handled different way than common string in options parsing in general.

I think norebind flag could be used as flag in existing serverarray
records, which I think always lookups domain before. It might be
optimized to fill norebind flag into serv_local flags, so it does not
have to loop via bind domains list again. It might be processed in
build_server_array just once per server reconfiguration. But I expect
usually just single or low number of rebind ok domains would be used, it
should not save too many comparisons. I guess not worth more complex
changes. But I made no_rebind change separate, because it never requires
any other part than serv_local offers.

I would include the flag anyway, just to have some value set to flags. I
have found it is not needed just after I send the patch.

>
> Should the send-buffers patch also touch dhcp.c, which also calls
> sendmsg? I'm not sure about adding code and complexity just to placate
> valgrind, especially as the code actually initialises all the published
> members, so the uninitialised data is presumably alignment padding?
I added memsetting msg just for additinal safety, removed that again.
Warning is actually emitted just because control_u structure having
random garbage and is part of msg used. I think it should be initialized
also for dhcp.c. Modified the change to set only control_u in all
references. Should be better than leaking actual memory from program in
any event.
>
> I'm also not sure about the structure re-arrangement. The code is more
> complex but how is it better?

I thought I already explained reasons, but found I never sent that
message. Sorry. Sent original one and adding something here.

I think it is better, because it works only with common available base
included in all structures. If it needs additional members, it has to
make check it is indeed there. Previous checks were somehow implicit or
hidden, if even in place. Makes obvious places where struct server
members are required. Makes it clear server is just specialized instance
of serv_local. I think it is much safer than having larger structure
pointer, which is not always as large as it seems. Should help in
readability in future.

It adds checks in forward to handle possibility serverarray member might
not be server struct. It is not clear to me whether lookup_domain
enforced such case. But it seems to me more people might forget to
ensure which kind it is, it won't let them in my proposal.

Cheers,

Petr

>> That is still unfixed even after commit
>> de372d6914ae20a1f9997815f258efbf3b14c39b.
>>
>> Made that change separate commit, rebased the previous change with small
>> changes.
>>
>> On 9/23/21 12:28, Simon Kelley wrote:
>>> On 22/09/2021 23:07, Petr Menšík wrote:
>>>> Good catch. A new bug #2006367 [1] were also reported on Fedora. It
>>>> seems to point to related structures and memory corruption in them. I
>>>> have no coredump to check it (yet), so mostly guessing.
>>>>
>>>> Juggling with type unsafe structures with few common members is quite
>>>> bad idea. I think those structures should contain common server_local
>>>> struct member at the start. They could pass pointer to it on every place
>>>> which needs working just with those common parts.
>>>>
>>>> On domain-match.c:677 is also suspicious memset. Its flags are not
>>>> directly related to allocated size. I think there might be a case, when
>>>> it overwrites more memory than allocated for the pointer. On line 696 it
>>>> may overwrite interface target even with flags SERV_4ADDR | SERV_6ADDR.
>>>> Both allow rewriting uid member when HAVE_LOOP is set, which is a
>>>> default. 
>>> SERV_IS_LOCAL is defined at the top of the file, and provides a simple
>>> check. If that's NOT true then the record is a complete server struct.
>>> If it is set then it's a struct serv_local, possibly with added IPv4 or
>>> IPV6 address, controlled by the SERV_4ADDR and SERV_6ADDR flags bits.
>>>
>>> /* If the server is USE_RESOLV or LITERAL_ADDRESS, it lives on the
>>> local_domains chain. */
>>> #define SERV_IS_LOCAL (SERV_USE_RESOLV | SERV_LITERAL_ADDRESS)
>>>
>>> With that information, it's fairly easy to see that the code is correct.
>>>
>>> The problem here was nothing to do with this code, the domain search
>>> code assumed that --server=/example.com/# would not be set along with
>>> --server=/example.com/#/<address> and when that was done, it wrongly
>>> returned BOTH server records, each if which has different lengths. The
>>> consumer of that set of records  assumed (as it was entitled to do) they
>>> were of the same type and length, hence the dereferencing of fields
>>> outside allocated memory.
>>>
>>>> I see many tricky corners without simple and readable checks
>>>> ensuring it always does what it should. I think char type enum would
>>>> definitely not hurt in common structure instead of this juggling with
>>>> flags. It would be much more clear what members are available. I think
>>>> default struct should be the smallest one and only retyped to bigger
>>>> struct, if some flag clearly indicated it is there. Preferred would be
>>>> separate type member.
>>> A major goal of this rewrite was to avoid wasting memory when there are
>>> 100,000 --address=/adserver.com/ lines in the configuration. Doing what
>>> you suggest would break that.
>>>> At first it should be fixed by minimal fix. I think constant sized
>>>> structure with some unused members would be far more safe. I think union
>>>> would be good candidate here. Its a pity we did not notice those issues
>>>> before release. I should spend some time on basic automated tests again.
>>>> I think dnsmasq it small, but needs more regular testing.
>>>>
>>> Agree entirely. I still have you test system in a git branch, and would
>>> like to progress it. It would have been nice to find the regression
>>> (removal) of --address=/#/..... before release.
>>>
>>>
>>> Cheers,
>>>
>>> Simon.
>>>
>>>
>>>
>>>> Cheers,
>>>>
>>>> Petr
>>>>
>>>> 1. https://bugzilla.redhat.com/show_bug.cgi?id=2006367
>>>>
>>>> On 9/16/21 16:31, Dominik DL6ER wrote:
>>>>> Addendum: Depending on the configuration, it can happen that the
>>>>> query is sent to another server that is configured to be used for
>>>>> an altogether different domain, e.g.
>>>>>
>>>>>> server=127.0.0.1#5353
>>>>>> server=::1#5353
>>>>>> rev-server=192.168.0.1/24,192.168.0.1
>>>>>> server=/fritz.box/192.168.0.1
>>>>>> server=/bo.net/#
>>>>>> address=/bo.net/#/
>>>>> resulting in "A bo.net" being sent to 192.168.0.1
>>>>>
>>>>> Something is definitely fishy here.
>>>>>
>>>>> Best,
>>>>> Dominikman/listinfo/dnsmasq-discuss

-- 
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemensik at redhat.com
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB




More information about the Dnsmasq-discuss mailing list