Opened 7 years ago

Closed 7 years ago

#8447 closed defect (fixed)

p_circ_id is a uint32_t, but we print it as a %d

Reported by: arma Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Take for example

Mar 10 05:08:15.000 [info] circuit_expire_old_circuits_serverside(): Closing circ_id -2147477157 (empty 62 secs ago)

This happens because p_circ_id is of type circid_t, which is a uint32_t.

There are a bunch of cases in the code where we print p_circ_id as a %d. I guess the overflow is extra likely because of the "use the first bit to decide which piece of the id-space you'll use" trick.

Child Tickets

Change History (7)

comment:1 Changed 7 years ago by arma

Could easily move this to 0.2.5 if we prefer. Assuming I'm right that it's just cosmetic?

comment:2 Changed 7 years ago by nickm

It's cosmetic in practice, but it's technically undefined behavior, so we should fix it.

At least, the way I read the C standard, it's undefined: integer arguments passed to a variable-arguments function as variable arguments are subject to the "default argument promotions", which mean that any thing smaller than int gets promoted to int. So if int is 64-bit, we're fined: uint32_t can get promoted to int without trouble. But if int is 32-bit, uint32_t stays where it is. The standard says:

If there is no actual next argument, or if type is not compatible with the type of the
 actual next argument (as promoted according to the default argument promotions), 
the behavior is undefined, except for the following cases:
     - one type is a signed integer type, the other type is the corresponding unsigned 
       integer type, and the value is representable in both types;

so I don't think we're strictly speaking in the clear.

In practice, this won't hurt in any way I can tell, but we shouldn't tolerate undefined behavior lightly.

I wonder why GCC isn't warning about this?

comment:3 Changed 7 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:4 Changed 7 years ago by nickm

In case anybody can work on this, I've started work on a fix in branch "bug8447". In order to make GCC warn in every case where we printf circid_t as %d, I've temporarily changed the type of circid_t to uint64_t. Once it compiles without warnings, I'll change it back.

If nobody else can do this, I'll try to finish it later today or tomorrow.

comment:5 Changed 7 years ago by nickm

Status: acceptedneeds_review

Branch "bug8447" should go in the next 0.2.4. Please review.

It should get squashed before merging it; it temporarily turned circid_t into uint64_t so that the compiler would warn me about printfs and logs.

comment:6 Changed 7 years ago by arma

I looked over the patch, and it looks ok. Assuming %u is indeed the right way to print these things, I think it should be fine.

"Bug not in any released Tor" -- ah ha, this is from the recent change from 2 to 4 bytes, you're right.

comment:7 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay. Squashing and merging. Thanks for the review!

Note: See TracTickets for help on using tickets.