search

paullouisageneau / libplum

Multi-protocol Port Mapping client library
Mozilla Public License 2.0
40 stars 5 forks source link

Fix handling protocol->init() failure, fix double-free #18

Closed past-due closed 4 months ago

past-due commented 5 months ago

If upnp_init() fails, state->impl has already been freed.

upnp_cleanup() then later accesses state->impl.

Caught by AddressSanitizer:

=================================================================
==12240==ERROR: AddressSanitizer: heap-use-after-free on address 0x000103203da0 at pc 0x00010073e018 bp 0x00016fe86770 sp 0x00016fe86768
READ of size 4 at 0x000103203da0 thread T1
    #0 0x10073e014 in upnp_cleanup+0x1e8 (libplum.0.4.0.dylib:arm64+0x3e014)
    #1 0x100714460 in reset_protocol+0x308 (libplum.0.4.0.dylib:arm64+0x14460)
    #2 0x10070eccc in client_run+0xd9c (libplum.0.4.0.dylib:arm64+0xeccc)
    #3 0x10070df1c in client_thread_entry+0x14 (libplum.0.4.0.dylib:arm64+0xdf1c)
    #4 0x1004615bc in _pthread_start+0x84 (/usr/lib/system/introspection/libsystem_pthread.dylib:arm64e+0x15bc)
    #5 0x10046ba9c in thread_start+0x4 (/usr/lib/system/introspection/libsystem_pthread.dylib:arm64e+0xba9c)

0x000103203da0 is located 0 bytes inside of 96-byte region [0x000103203da0,0x000103203e00)
freed by thread T1 here:
    #0 0x1008b6ce0 in wrap_free+0x98 (/usr/lib/clang/15.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:arm64e+0x52ce0)
    #1 0x10073dd58 in upnp_init+0xf78 (libplum.0.4.0.dylib:arm64+0x3dd58)
    #2 0x10070e818 in client_run+0x8e8 (libplum.0.4.0.dylib:arm64+0xe818)
    #3 0x10070df1c in client_thread_entry+0x14 (libplum.0.4.0.dylib:arm64+0xdf1c)
    #4 0x1004615bc in _pthread_start+0x84 (/usr/lib/system/introspection/libsystem_pthread.dylib:arm64e+0x15bc)
    #5 0x10046ba9c in thread_start+0x4 (/usr/lib/system/introspection/libsystem_pthread.dylib:arm64e+0xba9c)

previously allocated by thread T1 here:
    #0 0x1008b6ba4 in wrap_malloc+0x94 (/usr/lib/clang/15.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:arm64e+0x52ba4)
    #1 0x10073cf4c in upnp_init+0x16c (libplum.0.4.0.dylib:arm64+0x3cf4c)
    #2 0x10070e818 in client_run+0x8e8 (libplum.0.4.0.dylib:arm64+0xe818)
    #3 0x10070df1c in client_thread_entry+0x14 (libplum.0.4.0.dylib:arm64+0xdf1c)
    #4 0x1004615bc in _pthread_start+0x84 (/usr/lib/system/introspection/libsystem_pthread.dylib:arm64e+0x15bc)
    #5 0x10046ba9c in thread_start+0x4 (/usr/lib/system/introspection/libsystem_pthread.dylib:arm64e+0xba9c)

Thread T1 created by T0 here:
    #0 0x1008afb10 in wrap_pthread_create+0x54 (/usr/lib/clang/15.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:arm64e+0x4bb10)
    #1 0x1007102f8 in client_start+0x1cc (libplum.0.4.0.dylib:arm64+0x102f8)
    #2 0x10073352c in plum_create_mapping+0xa0 (libplum.0.4.0.dylib:arm64+0x3352c)
    #3 0x1000034d8 in main+0x374 (example:arm64+0x1000034d8)
    #4 0x19f01e0dc  (<unknown module>)

SUMMARY: AddressSanitizer: heap-use-after-free (libplum.0.4.0.dylib:arm64+0x3e014) in upnp_cleanup+0x1e8
==12240==ABORTING
past-due commented 4 months ago

I think the proper fix is to reset client->protocol to NULL if client->protocol->init() fails:

https://github.com/paullouisageneau/libplum/blob/c111e0a5af50fefc87417fa6e6d4b062406dd7cb/src/client.c#L317

Implemented this fix.

I also reset state->impl = NULL after freeing, everywhere that is done.

And fixed a possible double-free in http_perform_rec() (error: already frees buffer, but there were a few places that also did so before goto error).