Opened 7 years ago

Closed 7 years ago

#6853 closed defect (fixed)

Relay-flag voting code has undefined behaviour

Reported by: rransom Owned by: rransom
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: 023-da tor-auth
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

After Nick found and fixed #6833, the bughunter with many names pointed out that the following code still has undefined behaviour, even with j known to be less than the bit width of *flags_out:

          *flags_out |= (1<<j);

The problem is that 1 has type int, so on platforms where int only has 32 bits (i.e. almost all of them), this still tries to shift by more than the width of the type in one fell swoop.

This undefined behaviour is probably lurking in everything that touches this flags field, not just the parsing goo.

Marking as 0.2.4.x-only for now, but this should definitely be backported to a future 0.2.3-da branch.

Child Tickets

Change History (10)

comment:1 in reply to:  description Changed 7 years ago by rransom

Replying to rransom:

          *flags_out |= (1<<j);

Er, that's “vote_rs->flags |= (1<<p);” now.

comment:2 Changed 7 years ago by rransom

There are 2.5 sane ways to do this:

(1) Rename the flags field and audit every reference to it that the compiler finds. Then, in a fixup! commit, rename it back.

(2) Bite the bullet and implement and use a real bit-vector data structure, complete with runtime-variable length and dynamic allocation. (Also rename the flags field.)

(2') Implement a bit-field data structure, with length set at compile time. (Also rename the flags field.)

comment:3 Changed 7 years ago by asn

A person on IRC says that uint64_t f = (1<<31) gives 0xFFFFFFFF80000000 instead of wished 0x80000000, for all 32bit integer architectures.

comment:4 Changed 7 years ago by nickm

Status: newneeds_review

I did 1' : renamed flags to find every reference, then audited the code at all those points, then changed the name of flags back. There were only a small number of references in 3 files (one of which was test_dir.c), so it was pretty easy.

It turns out the case above is the only problem I could find in the code near these sites.

We should eventually do (2) or something like it.

I don't see any literal cases of (1<<31), though if somebody knows one, I'd like to fix it. (I did a grep for "<<", then grepped all those for "31".)

Please review branch "bug6853".

comment:5 Changed 7 years ago by nickm

Keywords: 023-da added

comment:6 in reply to:  4 Changed 7 years ago by rransom

Replying to nickm:

I did 1' : renamed flags to find every reference, then audited the code at all those points, then changed the name of flags back. There were only a small number of references in 3 files (one of which was test_dir.c), so it was pretty easy.

It turns out the case above is the only problem I could find in the code near these sites.

I repeated this search. Your branch looks good.

comment:7 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.3.x-final

Merged to master. Leaving open in 0.2.3.x-final in case there is a 0.2.3.x-da.

comment:8 Changed 7 years ago by nickm

Keywords: tor-auth added

comment:9 Changed 7 years ago by nickm

Component: Tor Directory AuthorityTor

comment:10 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final
Resolution: fixed
Status: needs_reviewclosed

There will be no 0.2.3.x-da.

Note: See TracTickets for help on using tickets.