search

python / cpython

The Python programming language
https://www.python.org/
Other
60.06k stars 29.09k forks source link

gh-101732: Modules/_ssl.c: use Y2038 compatible openssl function when available #118425

Closed kanavin closed 2 weeks ago

kanavin commented 3 weeks ago
encukou commented 2 weeks ago

Thanks. I see that the function was added in OpenSSL 3.3, released April 10. (It's not in Arch You'll probably know: is there a distro/container with OpenSSL 3.3, test this?)

It returns time_t. Could you use _PyLong_FromTime_t rather than PyLong_FromLongLong? (This function is currently not public, but that's another issue.)

I see that Arch Linux now has OpenSSL 3.3, so I should be able to test this in an Arch VM. Before I do that, please run your tests with _PyLong_FromTime_t.

kanavin commented 2 weeks ago

Thanks. I see that the function was added in OpenSSL 3.3, released April 10. (It's not in Arch You'll probably know: is there a distro/container with OpenSSL 3.3, test this?)

It returns time_t. Could you use _PyLong_FromTime_t rather than PyLong_FromLongLong? (This function is currently not public, but that's another issue.)

I see that Arch Linux now has OpenSSL 3.3, so I should be able to test this in an Arch VM. Before I do that, please run your tests with _PyLong_FromTime_t.

I've done that with both original and fixed (as you requested) version. The tests are run on a 32 bit system running in qemu with time set to 2050, certificates regenerated to not be expired (discussed elsewhere :), and openssl 3.3.0.

test_ssl fails without the patch and succeeds with it.

kanavin commented 2 weeks ago

Thread-sanitizer fail doesn't seem to be related? Hard for me to tell for sure.

encukou commented 2 weeks ago

Yes, doesn't seem related.

Please don't force-push to CPython PRs; the brand new commits need to be reviewed all over again. In a bigger PR it would be an issue :) You don't need to worry about the branch being up to date.

kanavin commented 2 weeks ago

Turns out I can't easily get a 32-bit system with system clock set past 2038 :) But on a 64-bit one, this passes, and I can't see anything wrong with the PR. So I'll merge.

If the system is using 32 bit time_t (which is the default in glibc), it will immediately collapse altogether. You need either a non-glibc alternative (e.g. musl), or everything needs to be rebuilt with -D_TIME_BITS=64, which as far as I know only the most recent release of Yocto does. Debian has plans, but I have no idea how far they are implemented.