Crash in Zonemaster::Engine::Nameserver

[mattias-p]

Zonemaster::Engine::Nameserver is tripped up by the Backend test suite in zonemaster/zonemaster-backend#299.

$ make test TEST_FILES='t/test01.t'
...
t/test01.t .. 1/? Can't call method "string" on unblessed reference at /home/mattiasp/local/lib/perl5/Zonemaster/Engine/Nameserver.pm line 190.
...

Update:

The problem is that the internal state of Zonemaster::Engine can be poisoned, causing Zonemaster::Engine->add_fake_delegation() to crash when called with valid input.

The poisoning is caused in this case by Zonemaster::Engine::Nameserver->preload_cache() at the interface level, and by Zonemaster::Engine::Nameserver::Cache->new() at a deep level. These procedures are called with invalid input, and instead of catching and reporting the error, they poison the internal state.

The invalid input should be caught before the internal state is poisoned so that error symptoms surface close to their source.

Here's a minimized test case. Zonemaster::Engine::Nameserver::Cache->new() returns normally even though it's arguments are invalid, and Zonemaster::Engine->add_fake_delegation() crashes even though it's arguments are valid.

use Zonemaster::Engine;

eval {
    print "In one corner of the code base:\n";
    print "Buggy code injecting junk into the cache.\n";
    my $cache = Zonemaster::Engine::Nameserver::Cache->new(
        {
            data => { "nv7L7BL/+Xv5gj+KmxkC/w" => "This is a string instead of a Zonemaster::Engine::Packet" },
            address => Zonemaster::Engine::Net::IP->new('2001:dc3::35')
        }
    );
    print "Successfully injected junk into cache.\n";
};
if ( $@ ) {
    print "Failure occurred close to the bug in the code. This is easy to debug.";
    print "The error message:\n";
    print $@;
}

print "In another corner of the code base:\n";
print "Perform important but unrelated calculations.\n";
my $count;
for ( 1..1000000 ) { $count++ }

eval {
    print "In a third corner of the code base:\n";
    print "Correct code that relies on the cache being consistent.\n";
    Zonemaster::Engine->add_fake_delegation(
        'example.com' => {
            'ns1.example.com' => ['192.0.2.1'],
        }
    );
    print "No problem. Happy times.\n";
};
if ( $@ ) {
    print "Failure occurred far away from the bug in the code. This is very difficult to debug.\n";
    print "The error message:\n";
    print $@;
}

Update 2020-09-10:

#498 updated the cache implementation, breaking the test code given here. So I've updated the test code to demonstrate that the original issue is still present in the code.

[matsduf]

Is t/test01.t from Engine or Backend?

[mattias-p]

The one in zonemaster/zonemaster-backend@6a363a3.

[mattias-p]

What happens is Zonemaster::Engine::preload_cache() is called with invalid cache data. That procedure should validate its input. Otherwise other parts of Zonemaster::Engine can't trust the format of the cache data, and invalid cache data can go unnoticed only to show up in unexpected places.

[matsduf]

The issue for zonemster-backend was resolved by correcting the test01.data file for zonemaster-backend in PR zonemaster/zonemaster-backend#299. See commit zonemaster/zonemaster-backend@516a5cb

@mattias-p, should this issue still be kept open?

[mattias-p]

@matsduf Yes, I updated the description with more details.

[sandoche2k]

@mattias-p Is this issue still relevant?

[mattias-p]

@matsduf, @sandoche2k I updated the test code with informational print lines to make it clearer. This issue has not been fixed.

[matsduf]

@mattias-p, is this still relevant?

[mattias-p]

@matsduf Yes it's relevant, but #778 fixes it. The fix is much smaller and easier than I initially expected, but that's largely because the problematic interface was updated and simplified after the issue was created.

[matsduf]

Will be fixed by #778.