Opened 4 months ago

Closed 3 weeks ago

#23954 closed defect (fixed)

Race condition in LOG_PROTOCOL_WARN

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-31
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

LOG_PROTOCOL_WARN currently calls get_options(). After #23952 is merged, it will call get_protocol_warning_severity_level(). The trouble is, both of these functions access a global variable without acquiring a lock... and LOG_PROTOCOL_WARN can be invoked from outside the main thread.

Let's fix this once #23952 and #23953 are merged.

Child Tickets

Change History (7)

comment:1 Changed 5 weeks ago by nickm

Branch bug23954 is ready for review. I used the atomic_counter_t structure here so we could get good performance on platforms with stdatomic support.

comment:2 Changed 5 weeks ago by nickm

Status: assignedneeds_review

comment:3 Changed 4 weeks ago by nickm

Keywords: review-group-31 added

comment:4 Changed 4 weeks ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision
  • Nitpick: I would wrap this in its own function like the init and cleanup functions:
+  {
+    int warning_severity = options->ProtocolWarnings ? LOG_WARN : LOG_INFO;
+    atomic_counter_exchange(&protocol_warning_severity_level,
+                            warning_severity);
+  }

Like a "set" function that wraps the exchange() so we have one function that is allow to set protocol_warning_severity_level so init_protocol_warning_severity_level() can also use it.

  • Quick thing also, I would document the reason why we use an atomic counter for that value that is at least explaining what the possible race is exactly:
+static atomic_counter_t protocol_warning_severity_level;

comment:5 Changed 3 weeks ago by nickm

Status: needs_revisionneeds_review

Okay; I've tried to resolve those. Better now?

comment:6 Changed 3 weeks ago by dgoulet

Status: needs_reviewmerge_ready

Both fixups are very nice!

Thanks. ACK.

comment:7 Changed 3 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

squashed and merged; thanks!

Note: See TracTickets for help on using tickets.