Opened 3 months ago

Closed 2 months ago

#29537 closed defect (fixed)

verify intptr_t round-trip through void *

Reported by: catalyst Owned by:
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: portability technical-debt nickm-merge, asn-merge
Cc: rl1987 Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: catalyst, teor Sponsor:

Description

Round-trip of intptr_t through void * isn't explicitly guaranteed by C99. There are implementation-defined behaviors for integer to void *, and vice versa. Only a round-trip of void * through intptr_t is guaranteed, not vice versa. We seem to depend on being able to store an integer in a void * from time to time, e.g., to store integers in a smartlist.

We should run some automated tests to make sure this works as we expect, at least until we stop needing this behavior. (See also #23714.)

Child Tickets

Change History (33)

comment:1 Changed 3 months ago by rl1987

Cc: rl1987 added

comment:3 Changed 3 months ago by rl1987

Status: newneeds_review

comment:4 Changed 3 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Status: needs_reviewneeds_revision

This is a good start! I'd suggest a few more things:

  • Let's not only use 42. Let's check this property for a bunch of suspicious values, including: 0..1024 and UINT_MAX-1024..UINT_MAX. That won't catch all possible weird systems, but it will give us a few more chances to find corner cases.
  • Let's not put this in test.c; probably, nothing should be in test.c.

comment:5 in reply to:  description ; Changed 3 months ago by rl1987

Replying to catalyst:

Round-trip of intptr_t through void * isn't explicitly guaranteed by C99. There are implementation-defined behaviors for integer to void *, and vice versa. Only a round-trip of void * through intptr_t is guaranteed, not vice versa.

Is that true though?

From http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf:

7.18.1.4 Integer types capable of holding object pointers
1 The following type designates a signed integer type with the property that any valid
pointer to void can be converted to this type, then converted back to pointer to void,
and the result will compare equal to the original pointer:
intptr_t
The following type designates an unsigned integer type with the property that any valid
pointer to void can be converted to this type, then converted back to pointer to void,
and the result will compare equal to the original pointer:
uintptr_t
These types are optional.

My reading of this is that it does guarantee the round-trip for all values between INTPTR_MIN and INTPTR_MAX provided that C implementation does have intptr_t. Or does C99 standard (which people are supposed to purchase from ISO) say differently?

comment:6 in reply to:  5 Changed 3 months ago by catalyst

Replying to rl1987:

Replying to catalyst:

Round-trip of intptr_t through void * isn't explicitly guaranteed by C99. There are implementation-defined behaviors for integer to void *, and vice versa. Only a round-trip of void * through intptr_t is guaranteed, not vice versa.

Is that true though?

From http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf:

7.18.1.4 Integer types capable of holding object pointers
1 The following type designates a signed integer type with the property that any valid
pointer to void can be converted to this type, then converted back to pointer to void,
and the result will compare equal to the original pointer:
intptr_t
The following type designates an unsigned integer type with the property that any valid
pointer to void can be converted to this type, then converted back to pointer to void,
and the result will compare equal to the original pointer:
uintptr_t
These types are optional.

My reading of this is that it does guarantee the round-trip for all values between INTPTR_MIN and INTPTR_MAX provided that C implementation does have intptr_t. Or does C99 standard (which people are supposed to purchase from ISO) say differently?

The quoted text guarantees to preserve values in conversions from void * to intptr_t back to void *. The text does not explicitly guarantee that conversions from intptr_t to void * back to intptr_t will preserve values. It's unlikely to be of practical importance at the moment, especially on platforms with a flat address space, but I've learned to never underestimate the cleverness of compiler optimizers.

For example, some values of intptr_t might not map to a valid void * ("trap representation"), or multiple intptr_t values might map to the same void * (making the mapping non-invertible).

comment:7 Changed 3 months ago by rl1987

Status: needs_revisionneeds_review

The following patch checks that we can put all valid values of int and unsigned int into void * and take them back. I suppose this is the ranges of number we care about in practice, as we may want to put signed and unsigned integer values into smartlist.

https://github.com/torproject/tor/pull/724

comment:9 Changed 3 months ago by asn

Reviewer: catalyst

comment:10 in reply to:  7 Changed 3 months ago by catalyst

Status: needs_reviewneeds_revision

Replying to rl1987:

The following patch checks that we can put all valid values of int and unsigned int into void * and take them back. I suppose this is the ranges of number we care about in practice, as we may want to put signed and unsigned integer values into smartlist.

https://github.com/torproject/tor/pull/724

Thanks! That seems to exhaustively check the entire int and unsigned int ranges. It might be better to check some limited ranges, as nickm suggested above. That way we can catch some likely failures without taking too much time.

The check doesn't have to be exhaustive because this is mostly an interim check until we stop doing this questionable thing in the future.

comment:11 Changed 2 months ago by rl1987

Status: needs_revisionneeds_information

By doing exhaustive check over the entire ranges of signed and unsigned integers we prove that intptr_t and void * interoperability is possible for all values in these ranges, instead of the ones we deem to be dangerous. I'm putting this code into test-slow to address the problem of it taking too long.

On what basis do we deem above ranges to be dangerous?

comment:12 Changed 2 months ago by nickm

I'm not claiming that those ranges are most dangerous; I'm claiming that if any integers fail to round-trip, it is likely that at least one of the integers in that range will fail to round-trip.

I don't think an exhaustive test is a good idea, especially since platforms where sizeof(int) > 8 do exist.

comment:13 Changed 2 months ago by nickm

err, I meant to say >= 8.

comment:14 Changed 2 months ago by rl1987

Status: needs_informationneeds_review

comment:15 Changed 2 months ago by teor

Reviewer: catalystcatalyst, teor

I did a quick review and made some suggestions,

I think testing negative ints is important, testing every bit is potentially useful and harmless, and putting the casts in another module improves the reliability of the test under optimisation (and is also harmless).

comment:16 Changed 2 months ago by teor

Status: needs_reviewneeds_revision

comment:17 Changed 2 months ago by rl1987

Are we doing the right thing here though? Intuitively, if we're writing code that a compiler might very reasonably optimize away, aren't we on a wrong path?

Perhaps we should create tickets to fix parts of tor codebase that rely on putting integers into void * one by one?

comment:18 in reply to:  description Changed 2 months ago by teor

Replying to rl1987:

Are we doing the right thing here though? Intuitively, if we're writing code that a compiler might very reasonably optimize away, aren't we on a wrong path?

Some of our existing code relies on behaviour that's not guaranteed by the standard. This ticket is about writing tests to make sure that our supported architectures do what we expect with this existing code. (Rather than changing pointer values in other ways.)

See the ticket description for details.

But these tests won't be any good to us if the casts are optimised away by the compiler. Much of our existing code avoids this optimisation by doing the casts in different functions in different modules. So the tests need to do the casts in different functions in a different module.

Perhaps we should create tickets to fix parts of tor codebase that rely on putting integers into void * one by one?

We already have #23714 to fix these issues. But we want to add a test now, and do the fixes later.

comment:19 Changed 2 months ago by rl1987

Status: needs_revisionneeds_review

comment:20 Changed 2 months ago by teor

Thanks!

The code looks good,
We're missing some function-level documentation.
And I have a question about the size of an int being smaller than or equal to void *.

comment:21 Changed 2 months ago by teor

(More precisely, the code seems to assume that int and intptr are the same size, but I don't think that's guaranteed.)

comment:22 Changed 2 months ago by teor

Status: needs_reviewneeds_revision

comment:23 Changed 2 months ago by rl1987

Status: needs_revisionneeds_review

comment:24 Changed 2 months ago by catalyst

Revisions look good to me, except for a minor changes file correction that I noted on GitHub, which might be fixable as part of the merge.

teor did you have any other comments to add?

comment:25 in reply to:  24 Changed 2 months ago by teor

Keywords: nickm-merge asn-merge added

Replying to catalyst:

Revisions look good to me, except for a minor changes file correction that I noted on GitHub, which might be fixable as part of the merge.

It's been fixed.

teor did you have any other comments to add?

Integer and pointer size checks belong in torint.h.
After that's done, please flip the ticket to merge_ready.

comment:26 Changed 2 months ago by teor

Status: needs_reviewneeds_revision

comment:27 Changed 2 months ago by rl1987

Status: needs_revisionmerge_ready

comment:28 Changed 2 months ago by asn

Seems like the PR has failed because of:

./src/test/ptr_helpers.h:No #ifndef/#define header guard pair found.
make[1]: *** [check-spaces] Error 1

likely the result of #29756.

Can you please fix the offending .h file and move back to merge_ready.

Last edited 2 months ago by asn (previous) (diff)

comment:29 Changed 2 months ago by asn

Status: merge_readyneeds_revision

comment:30 in reply to:  28 Changed 2 months ago by teor

Replying to asn:

Seems like the PR has failed because of:

./src/test/ptr_helpers.h:No #ifndef/#define header guard pair found.
make[1]: *** [check-spaces] Error 1

likely the result of #29756.

Can you please fix the offending .h file and move back to merge_ready.

Sorry about that, the header check is new and I just merged it yesterday.

comment:31 Changed 2 months ago by rl1987

Should be fixed in 4e6ba575a66108f38a55cffdb197352757f77d78 now.

comment:32 Changed 2 months ago by teor

Actual Points: 0.1
Points: 0.1
Status: needs_revisionmerge_ready

Once CI passes, we can merge to master.

comment:33 Changed 2 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Merged to master.

Thanks for your patience!

Note: See TracTickets for help on using tickets.