Bug: $typeId = $ans_header['type']; can be null

[Legendary4226]

Hello, I don't have an example that trigger the error at 100% chance, but it's linked to a preceding issue #29. And that's why I missed this check.

In DNSQuery.php:209, $typeId can be null :
$typeId = $ans_header['type'];
Then it throws an error at DnsQuery.php:355 :
'typename' => $this->types->getNameById($typeId),
because getNameById() ask for a non-nullable string.

This one is linked to the bug with the debug and the array that was a boolean (DNSQuery.php:209). If you remember the debug was calling $ans_header['type'] when it is a boolean. But in fact $ans_header['type'] is called a second time, so the same error happens. That's why it's linked with the error error at DNSQuery.php:355.

So both errors should be fixed to fully resolve this bug. Why it happens ? I don't have a better answer than this thread at stackoverflow :

Turns out that the warning appears when the input data is not correct, specifically if it expects more characters to unpack than the provided amount, in this case the unpack() function evaluates to false.

Have a good day :)

[purplepixie]

Thanks - I'll look into this!

[purplepixie]

Okay do you have an example query please @Legendary4226 where this occurs? I can see that the unpack failing would cause this but to decide if this is correct behaviour (the response is invalid) or not if possible I need to replicate. Either way the handling of it needs fixed.

[Legendary4226]

Right now I don't have a domain name and this issue don't happen all the time.

All I know is that I use the library to make a DNS query over all DNSTypes::$types except the NAME_ANY and another one that seems not to be implemented.

If you want to avoid releasing any breaking changes, you can do the same you did for #23 with the boolean to specifically handle this warning. In fact it's the same problem as I can't handle it easily. What I really need is to handle it as it happens randomly and frequently (each days) but not in a large amount.

[Legendary4226]

Ho and I just think of it. I found out TCP is more reliable and used than UDP. The majority of DNS servers use TCP only as the library use UDP by default. When I begin to use the library, all my queries was made using UDP and I had a lot of DNS servers giving no response.
Then I changed my code to do a first attempt with TCP. Then if an error occurs, try with UDP as fallback.

Maybe the error come from some bytes lost/changes during network exchanges.
So UDP is another reason why sometime the unpack() function may throw a warning.

[Legendary4226]

Hello, any update on how you plan to fix or allow to take control of this E_WARNING ?

[purplepixie]

I've been running tests to work out the best way to deal with this - it is an error response just want to ensure no breaking changes are introduced.

[purplepixie]

So; I have not been able to replicate this so can't be certain of the cause here but as you have seen this only on UDP I think it's going to be an error/incorrect/lost response which couldn't be read - there shouldn't be any conditions under which unpack() fails on any type of valid response.

In branch null-type-issue I have implemented the following behaviour:

If the result of unpack is a failure (null || false || !is_array) then a debug message is written and a new flag (responseException) is checked. If this is true (it's false by default) then an InvalidResponse exception is thrown, otherwise if it's false then an empty array is returned by readRecord(). This should not break any existing approaches while still allowing granular control.

@Legendary4226 apologies again for the delay; could you check and see if that solution would work for you? And feel free to try out the branched code.

I'll also probably add more points where InvalidResponse is thrown if the flag is set.

[Legendary4226]

Okay,
I've read the code and I'm afraid not adding a @ before the unpack() call will not change the issue.
Because the E_WARNING Warning: unpack(): Type N: not enough input, need 4, have 2 come from the unpack() function.

Except that, having a catch-able exception is all I need, in fact I will do nothing when this error occurs.

Thank you

[purplepixie]

Ah okay no problem - so I've added the @ suppression to the unpack() call and it also has the optional exception. I'll merge this back in now.

[purplepixie]

Merged and released in 2.3.5 - I'll leave this open in case there are any updates but hopefully this will resolve the issue. Docs also updated for setResponseException and InvalidResponse exception.

[Legendary4226]

Hello! The new release fixed the bug, thank you :)

A little last thing is that the new error is not indicated on the PHP doc @throws of the query() function.