Opened 8 years ago

Closed 8 years ago

#7305 closed defect (fixed)

enum variables with defined bit width

Reported by: ultramage Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: msvc tor-client
Cc: Actual Points:
Parent ID: #7754 Points:
Reviewer: Sponsor:

Description

The signedness of an enum type according to the C standard is implementation-defined, and thus shouldn't be relied on. However, the code implicitly assumes it's unsigned (a gcc-specific thing?), and uses constructs like this:

typedef enum {
  ADDR_POLICY_ACCEPT=1,
  ADDR_POLICY_REJECT=2,
} addr_policy_action_t;

addr_policy_action_t policy_type:2;

What happens to the above case with the MSVC compiler is that it treats the enum type as signed, and writing a '2' makes two's-complement math kick in, so '2' becomes '-2'. One unit test fails because of this, which is how I noticed it. The consequence is that Tor's state can easily destabilize, and impossible execution paths like this could occur:

test = 2;
assert( test == 2 ); // triggers

There are workarounds, like wrapping the enum in a struct (very ugly), or using an integer type for storage (loss of type info). Ideally stop using bit packing entirely in memory-only structures, and serialize to integer types of exact size when crafting packets (an enum's size is variable in gcc, fixed in msvc).

Here's a list of all offending places for patterns ":N" and ": N", N=1..9:

  • circ_id_type_t circ_id_type:2;
  • addr_policy_action_t policy_type:2;
  • path_state_t path_state : 2;
  • addressmap_entry_source_t source:3;
  • } state : 3;
  • } dir_spool_src : 3;
  • saved_location_t saved_location : 3;

Child Tickets

Change History (5)

comment:1 Changed 8 years ago by nickm

Keywords: tor-client added
Milestone: Tor: 0.2.4.x-final

Ideally stop using bit packing entirely in memory-only structures, and serialize to integer types of exact size when crafting packets

I think that the "store these as unsigned rather than as enum" approach is far more reasonable. (We never serialize a struct, I hope, and the reason we bit-pack these things is to save RAM. In some cases, as for circuits or policies, we allocate a really huge number of these things.)

This could go either 0.2.3 or 0.2.4, but I'm leaning 0.2.4 since it's a nontrivial fix and MSVC is a semi-supported build environment for 0.2.3.

comment:2 Changed 8 years ago by arma

agree that it looks like an 0.2.4 thing

comment:3 Changed 8 years ago by nickm

Parent ID: #7754

comment:4 Changed 8 years ago by nickm

Status: newneeds_review

This is in the 024_msvc branch.

comment:5 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged a fix for this in b998431a33db2b. Opened #7977 for the "make autoconf detect it" thing.

Note: See TracTickets for help on using tickets.