Opened 5 weeks ago

Last modified 4 weeks ago

#31735 new defect

Exit and join all threads, before destroying any mutexes in the main thread

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: diagnostics
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by teor)

Otherwise, the mutex could be locked by another thread, and destroying a locked mutex triggers undefined behaviour.

Part of #31614.

Updated to add:

Instead of exiting and joining threads, we can initialize the mutex once, and never destroy it. If we use a sentinel value, like log_mutex_initialized, then we won't re-initialize the mutex when we re-initialize everything else.

Child Tickets

Change History (8)

comment:1 Changed 5 weeks ago by teor

Keywords: consider-backport-after-042-stable consider-backport-if-needed 042-should 035-backport-maybe 040-backport-maybe 041-backport-maybe regression BugSmashFund removed
Milestone: Tor: 0.4.2.x-finalTor: unspecified
Parent ID: #31614
Version: Tor: 0.3.5.1-alpha

This is a complex change, and we probably don't need to do it any time soon.

comment:2 Changed 5 weeks ago by teor

We should probably also check errors when calling pthred_mutex_destroy(), and its callers.

comment:3 Changed 4 weeks ago by teor

For pthreads and winthreads, we need to:

  • for each thread:
    • release any mutexes held by the thread
    • exit the thread
    • join the thread
  • for each global mutex:
    • destroy the mutex

Then we can re-initialise tor.

comment:4 Changed 4 weeks ago by teor

From https://trac.torproject.org/projects/tor/ticket/31736#comment:6

There's some code that could be used for an integration test, but it would need work. See the configure option --enable-restart-debugging and the corresponding environment variable TOR_DEBUG_RESTART, both added in 97d9ba2380e0c894a1b611bdb4f35d0fe98a837a.

comment:5 Changed 4 weeks ago by teor

Description: modified (diff)

Instead of exiting and joining threads, we can initialize the mutex once, and never destroy it. If we use a sentinel value, like log_mutex_initialized, then we won't re-initialize the mutex when we re-initialize everything else.

comment:6 Changed 4 weeks ago by nickm

This sounds reasonable. Maybe we should have a pattern for this, so that we don't need to replicate the code in a slightly different way everywhere that we have a statically allocated mutex.

comment:7 Changed 4 weeks ago by nickm

For example, we could have:

typedef struct static_mutex_t {
    pthread_mutex_t *m;
    bool initialized;
};

But when I look at this, I'm not sure whether checking initialized is actually any better than just checking whether the mutex itself is NULL.

comment:8 Changed 4 weeks ago by nickm

Ah, I get it. Your suggestion is closer to:

typedef struct static_mutex_t {
    pthread_mutex_t m;
    bool initialized;
};

That would make sense.

Note: See TracTickets for help on using tickets.