verify intptr_t round-trip through void *
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 (moved).)
Activity
changed milestone to %Tor: 0.4.1.x-final
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.
Trac:
Status: needs_review to needs_revision
Milestone: Tor: unspecified to Tor: 0.4.1.x-final-
Replying to catalyst:
Round-trip of
intptr_tthroughvoid *isn't explicitly guaranteed by C99. There are implementation-defined behaviors for integer tovoid *, and vice versa. Only a round-trip ofvoid *throughintptr_tis 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_MINandINTPTR_MAXprovided that C implementation does haveintptr_t. Or does C99 standard (which people are supposed to purchase from ISO) say differently?Replying to rl1987:
Replying to catalyst:
Round-trip of
intptr_tthroughvoid *isn't explicitly guaranteed by C99. There are implementation-defined behaviors for integer tovoid *, and vice versa. Only a round-trip ofvoid *throughintptr_tis 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_MINandINTPTR_MAXprovided that C implementation does haveintptr_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 *tointptr_tback tovoid *. The text does not explicitly guarantee that conversions fromintptr_ttovoid *back tointptr_twill 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_tmight not map to a validvoid *("trap representation"), or multipleintptr_tvalues might map to the samevoid *(making the mapping non-invertible).The following patch checks that we can put all valid values of
intandunsigned intintovoid *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
Trac:
Status: needs_revision to needs_reviewReplying to rl1987:
The following patch checks that we can put all valid values of
intandunsigned intintovoid *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
intandunsigned intranges. 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.
Trac:
Status: needs_review to needs_revisionBy doing exhaustive check over the entire ranges of signed and unsigned integers we prove that
intptr_tandvoid *interoperability is possible for all values in these ranges, instead of the ones we deem to be dangerous. I'm putting this code intotest-slowto address the problem of it taking too long.On what basis do we deem above ranges to be dangerous?
Trac:
Status: needs_revision to needs_information-
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.
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).
Trac:
Reviewer: catalyst to catalyst, teorReplying 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 (moved) to fix these issues. But we want to add a test now, and do the fixes later.
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.
Trac:
Keywords: portability technical-debt deleted, asn-merge, portability technical-debt nickm-merge added
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 1likely the result of #29756 (moved).
Can you please fix the offending .h file and move back to merge_ready.
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 (moved).
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.
mentioned in issue #29958 (moved)
moved to tpo/core/tor#29537 (closed)