Repository navigation
pyhash's siphash24 assumes alignment of the data pointer #72242
Description
Activity
pyhash's siphash24 assumes alignment of the data pointer, casting a void pointer (src) to an uint64_t pointer, increasing the required alignment from 1 to 4 bytes. That's invalid code. siphash24 can't assume that the pointer to the data to hash is 4-byte aligned.
Seen as a bus error trying to run a ARM32 binary on a AArch64 kernel.
./python -c 'import datetime; print(hash(datetime.datetime(2015, 1, 1)))'
the datetime type is defined as
#define _PyTZINFO_HEAD \ PyObject_HEAD \ Py_hash_t hashcode; \ char hastzinfo; /* boolean flag */ typedef struct { _PyTZINFO_HEAD unsigned char data[_PyDateTime_DATE_DATASIZE]; } PyDateTime_Date;
and data is used to calculate the hash of the object, not being 4 byte aligned, you get the bus error. Inserting three fill bytes, are making the data member 4-byte aligned solves the issue, however introducing an ABI change makes the new datetime ABI incompatible, and we don't know about the alignment of objects outside the standard library.
The solution is to use a memcpy instead of the cast to uint64_t, for now limited to the little endian ARM targets, but I don't see why the memcpy cannot always be used on little endian targets instead of the cast.
Good catch! I had trouble with the data structures in the TZ module before.
I'm fine with memcpy() on just ARM platforms as a temporary workaround. Let's discuss the issue another time. Right now I'm busy with ssl improvements for 3.6.0b1.
I believe the unaligned memory access configure check is supposed to prevent siphash from being used, so we might look into why that's not working.
IMO, though, we should just require alignment for the argument to _PyHash_Bytes. It's private after all.
I don't like that configure check, because it depends on the kernel being used at runtime. For many architectures you can define in the kernel if the kernel should allow unaligned accesses or not. Sure this is not an issue for linux distro builds, but might be unexpected for third party builds.
ztane commented
on Sep 11, 2016 ztanemannequinMannequinThere is no need to ifdef anything, the memcpy is the only correct way to do it. As memcpy is also a reserved identifier in C, the compiler can and will optimize this into a 64-bit access on those platforms where it can be safely done so (x86 for example), e.g. GCC compiles
uint64_t func(char *buf) { uint64_t rv; memcpy(&rv, buf+3, sizeof(rv)); return rv; }
into
movq 3(%rdi), %rax retOn Linux 64-bit ABI.
updated patch that always used memcpy for the little endian case.
I'm a bit worried that the patch might slow down the general case of SipHash24. When I was working on SipHash24 I made sure that the general case in PyBytes_Object and PyUnicode_Object are fast and always aligned. Do all compilers optimize that case? For MSVC we still have a specialized Py_MEMCPY() variant in pyports.h.
I can see three more ways to fix the issue:
- Have two loops, one for the aligned case with memcpy() and one for the unaligned case w/o memcpy()
- Add a special variant of _le64toh() for PY_LITTLE_ENDIAN on ARM and use the current variant on X86_64.
- Make it illegal to call _Py_HashBytes() with non-aligned pointer and require the caller to provide an aligned buffer. It's easy for datetime but requires an extra buffer memoryview. Memoryview already uses a buffer for all but single-strided C contiguous views. We can easily add another case for non-aligned buffers.
skrah commented
on Sep 13, 2016 skrahmannequinMannequinFWIW, MSVC optimizes memcpy:
http://bugs.python.org/issue15993
The pgo issue has been fixed according to Steve Dower.
a variant of the patch that keeps the parameter types of _le64toh.
I can check, if the memcpy is optimized away. As an alternative, we could use __builtin_memcpy. That is available for clang as well (would have to check icc).
I believe the unaligned memory access configure check is supposed to
prevent siphash from being used, so we might look into why that's not
working.IMO, though, we should just require alignment for the argument to
_PyHash_Bytes. It's private after all.If I understand it correctly, the hash value differs depending on the kernel configuration when the python binary is built, leading to different pickle objects which cannot be shared, making them incompatible . I think the safest thing would be to remove the hash make the selection of the hash method unconditional, and to make this hash function working for all cases.
18 remaining items
gco commented
on Jan 28, 2017 gcomannequinMannequin32-bit and 64-bit SPARC ABIs have 64-bit integer data types.
SPARC, like many RISC architectures, also has natural alignment requirements. Attempting to dereference a pointer to a 4-byte-sized object requires 4-byte alignment, for example. 2-byte-sized objects require 2-byte alignment. 8-byte-sized objects require 8-byte alignment.
siphash24 is encountering this bug on modern SPARC (32-bit ABI currently, haven't tried compiling as 64-bit yet). The code simply is not portable.
Benjamin's patch gets the failing self-test (test_plistlib) to pass as well as the simple test case in msg275493 above.
I agree with Stefan and Serhiy. Unaligned memoryviews shouldn't trigger a copy when hashing.
DerDakon commented
on Mar 26, 2018 DerDakonmannequinMannequinSo, what is the problem with this? Either the compiler knows that unaligned accesses are no problem and optimizes them away anyway, or it is kept because it would crash otherwise. I can confirm that no sparc version >= 3.5 (have not tried older) survives the test suite on Gentoo Sparc (64 bit kernel, 32 bit userspace) without memcpy().
skrah commented
on May 13, 2018 skrahmannequinMannequinMSVC optimizes memcpy() to an assignment, sometimes too well (pgo): https://bugs.python.org/issue15993
But that is fixed long ago, so I also think that the memcpy() approach is best.
Indeed the memcpy() approach is the common idiom in such situations, and sounds like the right thing.
JeffreyWalton commented
on Jul 21, 2018 JeffreyWaltonmannequinMannequinI know this is a bit late but I wanted to share...
OpenCSW has a build farm with Solaris machines and Sparc hardware. The farm provides x86 and Sparc machines with Solaris 9 through 11.
I believe OpenCSW operates in the same spirit as GCC compile farm. They welcome open source developers and upstream maintainers to help ensure packages build and run on Solaris machines.
You can read about it at
https://www.opencsw.org/extend-it/signup/to-upstream-maintainers/ .If Python is performing memory access patterns as discussed in the report then it would probably benefit the project to test on a Sparc machine with Solaris 11.
I would say that Python no longer officially supports sparc and solaris
because of the lack of volunteer.I see that a fix has been pushed. I'm not sure why this issue is still open, so I close it.
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields: