Bug 4209 - Crash in libunbound when called from getdns
Crash in libunbound when called from getdns
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
1.8.1
x86_64 FreeBSD
: P5 critical
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-22 16:03 CET by Philip Homburg
Modified: 2018-11-22 16:08 CET (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Homburg 2018-11-22 16:03:23 CET
Running in valgrind I got the following stack trace:

==60485== Invalid read of size 8                                               
==60485==    at 0x5F5CAB8: sldns_buffer_at (sbuffer.h:358)                     
==60485==    by 0x5F5CAB8: sldns_buffer_begin (sbuffer.h:370)                  
==60485==    by 0x5F5CAB8: libworker_event_done_cb (libworker.c:662)           
==60485==    by 0x5F808B5: mesh_state_cleanup (mesh.c:744)                     
==60485==    by 0x5F7F365: mesh_state_delete (mesh.c:795)                      
==60485==    by 0x5F7EF10: mesh_delete_helper (mesh.c:270)                     
==60485==    by 0x5F7EF10: mesh_delete (mesh.c:282)                            
==60485==    by 0x5F5B3BC: ??? (libworker.c:86)
==60485==    by 0x5F5B365: libworker_delete_event (libworker.c:116)
==60485==    by 0x5F584EA: ub_ctx_delete (libunbound.c:302)                    
==60485==    by 0x4E6EBDF: getdns_context_destroy (context.c:1814)              ==60485==    by 0x470F59: getrrsetbyname (getrrsetbyname-getdns.c:285)
==60485==    by 0x44AB8F: verify_host_key_dns (dns.c:232)
==60485==    by 0x4180C1: verify_host_key (sshconnect.c:1322)
==60485==    by 0x419C1C: verify_host_key_callback (sshconnect2.c:98)          
==60485==  Address 0x18 is not stack'd, malloc'd or (recently) free'd          

It seems that mesh_state_cleanup passes a NULL buffer to a callback. Later libworker_event_done_cb deferences the null pointer.
Comment 1 Wouter Wijngaards 2018-11-22 16:08:18 CET
Hi Philip,

Yes that should not happen.  Added a NULL check to that routine so it does not reference it.  Thanks for the report!

Best regards, Wouter

Index: libunbound/libworker.c
===================================================================
--- libunbound/libworker.c	(revision 4970)
+++ libunbound/libworker.c	(working copy)
@@ -657,8 +657,8 @@
 			sec = 1;
 		else if(s == sec_status_secure)
 			sec = 2;
-		(*cb)(cb_arg, rcode, (void*)sldns_buffer_begin(buf),
-			(int)sldns_buffer_limit(buf), sec, why_bogus, was_ratelimited);
+		(*cb)(cb_arg, rcode, (buf?(void*)sldns_buffer_begin(buf):NULL),
+			(buf?(int)sldns_buffer_limit(buf):0), sec, why_bogus, was_ratelimited);
 	}
 }