getdnsapi / getdns

A modern asynchronous DNS API https://getdnsapi.net/
Other
467 stars 126 forks source link

Memory Leak #466

Closed doublez13 closed 4 years ago

doublez13 commented 4 years ago

I've been testing a python DNS library I'm writing against stubby, and it looks like there's a 12k memory leak in getdns if an invalid request is sent that has the two high bits set 0xC0 in the query portion

#Python2 script to cause leak
import socket

conn = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
conn.connect(('127.0.0.1', 53))

unimportant_stuff = "000000000000"
query = chr(192)+'00000' #affected by 192 and above
packet = unimportant_stuff + query

conn.send(packet)
Valgrind:
==10454== LEAK SUMMARY:
==10454==    definitely lost: 12,288 bytes in 3 blocks
hanvinke commented 4 years ago

I am certainly not an an expert programmer, but are you sure this is a real problem? Since it is not possible in the UDP protocol to identify missing packets. And I think not even reliable with Stubby on top of the protocol and hoping that with every request packet this will indeed result in a fixed number of response packets. But maybe I am wrong.

doublez13 commented 4 years ago

The issue seems to come from the use of invalid requests using name compression. I'm still investigating. Not a big issue, but nonetheless it's pretty easy to leak 12k of memory over and over using a specially crafted request.

wtoorop commented 4 years ago

Thanks. I agree @doublez13 , stubby/getdns should not leak or crash or do anything it shouldn't with crafted packets. I've put this on my TODO list for this week.

wtoorop commented 4 years ago

Just for my own record... it is leaking upstream.tcp.read_buf , but I'm not sure where exactly yet as the read_buf gets transferred to netreq (which should dispose of it once it is destroyed).

doublez13 commented 4 years ago

I'm unable to reproduce the leak after applying your patches! Closing the bug.