Fix crashes in cupsDNSSDDelete() when Avahi fails to initialize.

[Abd002]

cupsDNSSDNew() calls cupsDNSSDDelete() to clean up when Avahi initialization fails. The problem is that some cleanup calls in cupsDNSSDDelete() were made even when the related fields had not been initialized yet.

This could cause three different problems:

• avahi_domain_browser_free(dnssd->dbrowser) was called when dbrowser was NULL. Avahi's _free() functions don't check for NULL themselves — they assert forward and abort the process if the caller doesn't:

  avahi_domain_browser_free(NULL) → browser.c:245: Assertion `b' failed. Aborted
  

  This is the one that actually crashed a real running application, which is what led to digging into this code path in the first place.

• cupsThreadCancel(dnssd->monitor) and cupsThreadWait(dnssd->monitor) were called when monitor was 0 because the monitor thread had not been created. This causes pthread_cancel(0) to segfault.

• When avahi_client_new() failed, its failure path freed dnssd->poll and then called cupsDNSSDDelete(), which freed the same dnssd->poll again, causing a double-free (free(): double free detected in tcache 2). cupsDNSSDDelete()'s own cleanup then reaches:

  avahi_simple_poll_free(NULL) → simple-watch.c:368: Assertion `s' failed. Aborted
  

The fix checks that dbrowser and monitor are valid before cleaning them up, using the same pattern already used elsewhere in cupsDNSSDDelete(). The manual avahi_simple_poll_free(dnssd->poll) in the avahi_client_new() failure path is also removed, since cupsDNSSDDelete() already handles freeing dnssd->poll.

[michaelrsweet]

This isn't a valid test - pthread_t doesn't have a defined "uninitialized" value (IMHO a big oversight in POSIX) and isn't a simple type.

[michaelrsweet]

I am OK with the concept of these changes, but you can't actually compare a pthread_t to anything directly like this (it is an opaque type).

[Abd002]

But cupsDNSSDNew() already does the same comparison. Both dnssd.c:856 and dnssd.c:905 test (dnssd->monitor = cupsThreadCreate(...)) == 0, and that 0 is CUPS_THREAD_INVALID which is exactly what cupsThreadCreate()returns on the POSIX branch whenpthread_create()` fails. So the existing code already depends on that comparison working.

I do understand your point that a pthread_t can't portably be compared to anything directly, and I'm happy to add a bool flag recording whether the monitor thread was created and gate the cleanup on that instead.

But since libcups has a single POSIX branch that treats every POSIX platform the same way, and CUPS_THREAD_INVALID is defined as (cups_thread_t)0, it looks to me like that assumption is already baked in. Am I missing something?

[Abd002]

I think it would be cleaner to eventually separate cupsThreadCreate() success/failure from the resulting thread handle, similar to how pthread_create() handles it. That way we don't need to rely on a special value of cups_thread_t to indicate failure.

[michaelrsweet]

I think it would be cleaner to eventually separate cupsThreadCreate() success/failure from the resulting thread handle,

Agreed, but we can't "break" this API until v4.0 at this point... :/

Maybe I can just introduce a cupsThreadCreate2 API that "does the right thing" and then wrap it to implement cupsThreadCreate...

The current code relies on pthread_t being an integer or pointer type, and defines a CUPS_THREAD_INVALID value ((cups_thread_t)0). In the interests of minimum code change right now, I suggest that you just substitute the 0 comparison for CUPS_THREAD_INVALID constant. I've pushed that part of the change to "dnssd.c".

[Abd002]

Thanks! I've updated the change to use CUPS_THREAD_INVALID and rebased it onto master.

Also, I'd be interested in working on the cupsThreadCreate2 API you mentioned for v4.0 after I finish and submit my GSoC work. If you're okay with that, I'd be happy to take it up when the time comes.