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

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


Hi Simon,

I made attempt to move common local entries to only local struct. It
then requires explicit retyping on every place where members of struct
server needs to be updated. Makes it simpler to search all places where
full fledged server is required. I am a bit uncertain how should first
and last indexes to arrayserver should work. It seems to me they have to
be server, not SERV_IS_LOCAL, because only that has first and last.
Items in between can be anything, but first and last are not members of
serv_local.

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.
Moved it to dnsmasq.h, because it needs to be checked from multiple places.
>
> /* 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 still think some combinations should produce either warning or error.
I think Dominik's example documents it fine:

server=/bo.net/#
address=/bo.net/#/

One of those lines would be ignored, unless I am not getting it correctly.

>
>> 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.
I understand what was reason for current state. But I consider wrong to
use the biggest structure to be used in structures, where something
smaller is actually allocated. It is hiding the problem and makes
problematic access difficult to locate. Unless we can be always sure
only the part actually allocated can be accessed, union would be better.
I think it is safer to expand to bigger structure when we know it should
be there. I tried to convert it to shared minimum.
>> 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,
>>> Dominik

-- 
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemensik at redhat.com
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Initialize-send-buffers-before-using-them.patch
Type: text/x-patch
Size: 1532 bytes
Desc: not available
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20210925/3dbc15bd/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Separate-limited-serv_local-from-full-struct-server.patch
Type: text/x-patch
Size: 20542 bytes
Desc: not available
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20210925/3dbc15bd/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Make-no_rebind-list-explicitly-serv_local.patch
Type: text/x-patch
Size: 2893 bytes
Desc: not available
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20210925/3dbc15bd/attachment-0005.bin>


More information about the Dnsmasq-discuss mailing list