Opened 4 years ago

Closed 4 years ago

#19196 closed defect (fixed)

[prop250] Shared random version parsing fails on 32 bit

Reported by: teor Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, regression
Cc: Actual Points:
Parent ID: #16943 Points:
Reviewer: Sponsor:

Description

In dgoulet's srv-testing-2, git 0234595:

The original patch to sr_parse_commit was:

-  version = (uint32_t) tor_parse_long(value, 10, 1, UINT32_MAX, NULL, NULL);
+  version = (uint32_t) tor_parse_ulong(value, 10, 1, UINT32_MAX, NULL, NULL);

Since this is a regression, it's worth noting that the unit test shared-random/sr_commit tests version parsing as part of commit parsing. It will fail on 32 bit platforms if we make this mistake again. (I can't see any way to make it fail on 64-bit platforms - using fixed-size integers with a function that takes platform-dependent-size integers is inherently error-prone, see #19063.)

Also, since this is a regression, can you please make sure that no other patches from the last few comments in #16943 were dropped?

Child Tickets

Change History (1)

comment:1 Changed 4 years ago by dgoulet

Resolution: fixed
Status: newclosed

Oh my... this is a mistake I made in the commit that changed reveal_num and version to a different size.

prop250: Change reveal_num to uint64_t and version to uint32_t

So that wasn't dropped from the last #16943 review but rather introduced by me by mistake. Apologize.

I've fixed it in the main branch under review. As for how to prevent that, running the tests on 32bit and #19063 as you said sounds like what we need to do.

Note: See TracTickets for help on using tickets.