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

[past-due]

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

[paullouisageneau]

Good catch, but this is not how the issue should be fixed. The actual problem is that cleanup must not be called if init fails, precisely because the state is not initialized in that case. The same issue probably exists for other protocols like with pcp_init() and pcp_cleanup().

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

libplum/src/client.c

Line 317 in c111e0a

err = client->protocol->init(&client->protocol_state);

[past-due]

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

libplum/src/client.c

Line 317 in c111e0a

err = client->protocol->init(&client->protocol_state);

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).

[paullouisageneau]

It looks good, thank you for the fixes!