Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#13085 closed defect (fixed)

[patch] tor control connection event mask (32 bits) is too small for events (33 events)

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: 0.2.5.5-alpha
Severity: Keywords: tor-relay, needs-test
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorR

Description

In the tor git sources in early September 2014, the tor control connection event mask (or.h 1740):
struct control_connection_t { ... uint32_t event_mask; ... }

is too small to contain all of the 33 listed events (control.h 158):
#define EVENT_MAX_ 0x0021

This makes the following code undefined for events 32 & 33 (control.c 585):
if (control_conn->event_mask & (1<<event)) {

The attached patch addresses this issue by making event_mask uint64_t, and casting to uint64_t before the left shift. It also updates the comment to note the upper bound of ((uint64_t)1)<<63.

As far as I can tell, the impact of this issue was to ignore or confuse with other event types:
#define EVENT_TRANSPORT_LAUNCHED 0x0020
#define EVENT_HS_DESC 0x0021

FYI - this error was discovered using a tor built with:
clang -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error -ftrapv

Version: tor 0.2.6.?-alpha git 54348201f7cce9c0c01e9d4835714a2fec55c67c

Child Tickets

Attachments (1)

control-event-mask-over-shift.patch (2.9 KB) - added by teor 5 years ago.
Use event_mask_t (64 bits) for all event mask operations

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by teor

Use event_mask_t (64 bits) for all event mask operations

comment:1 Changed 5 years ago by teor

My previous patch missed some 32-bit event masks in control.c.
The latest patch standardises all event_mask operations on event_mask_t within control.c.

comment:2 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-final
Resolution: fixed
Status: newclosed

Applied; thanks!

comment:3 Changed 5 years ago by arma

Keywords: SponsorR added

comment:4 Changed 5 years ago by teor

Keywords: needs-test added
Resolution: fixed
Status: closedreopened

I'll do a simple unit test for this.

comment:5 Changed 5 years ago by teor

Owner: set to teor
Status: reopenedassigned

comment:6 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

The unit test would be for 0.2.6; we don't usually backport those.

comment:7 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

comment:8 Changed 5 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.6.x-final
Resolution: fixed
Status: assignedclosed

5 months without unit test; re-closing. The test isn't going into 0.2.6 at this point.

comment:9 Changed 5 years ago by teor

Note: unit tests created in #15431

comment:10 Changed 4 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR
Note: See TracTickets for help on using tickets.