More fixes and improvements

[sjaeckel]

Checklist

• documentation is added or updated
• tests are added or updated
• if this fixes something: added a Fixes: tag to the commit message

[karel-m]

Do we actually still need sha1_x86_init and sha1_c_init when they are both equal to sha1_init?

[sjaeckel]

Do we actually still need sha1_x86_init and sha1_c_init when they are both equal to sha1_init?

Nope, they could be defined to sha1_init! Will do that :) Thanks!

[karel-m]

similarly sha224_x86_init / sha256_x86_init vs sha224_c_init / sha256_c_init

[MarekKnapek]

Maybe also unify the *_done functions. As there is no x86specific logic inside them. They do call the *_process functions which might get dispatched to portable or x86 version, but themselves do not contain any added smart-ness over the portable version.

[MarekKnapek]

This smells like undefined behavior to me. The struct hash_state or more precisely union hash_state has some data members (such as int or ulong32 or ulong64 or void*) that have alignment requirement other than 1. By this cast you break this alignment contract. This is not allowed in C and is considered undefined behavior.

This is the reason why I created a new data type ugly_struct in my #761. I could create two variants, one is:

struct ugly_struct
{
    char padding[x];
    hash_state state;
};

the other is:

struct ugly_struct
{
    char padding[x];
    sha1_state state;
};

I choose the second one. Because other members of the union could unintentionally increase the alignment.

The difference between my approach and your approach is that in your pursuit of un-alignment-ness of hash state, you also break the alignment of fundamental data types. In my pursuit of un-alignment-ness of hash state, I do not break the alignment of fundamental data types, only the hash state as whole.

[MarekKnapek]

Build with UBSan and get this result. tests/cipher_hash_test.c:17:12: runtime error: store to misaligned address 0x7ffcec3d0b71 for type 'hash_state *' (aka 'union Hash_state *'), which requires 8 byte alignment

[sjaeckel]

This smells like undefined behavior to me.

Absolutely.

This is the reason why I created a new data type ugly_struct

Unfortunately this won't work to demo the fixup of misaligned buffers, that's why I implemented it as I did.

$ pahole -C ugly_struct demos/small.o                                                                           
struct ugly_struct {
        char                       padding[1];           /*     0     1 */

        /* XXX 7 bytes hole, try to pack */

        hash_state                 state;                /*     8   856 */

        /* size: 864, cachelines: 14, members: 2 */
        /* sum members: 857, holes: 1, sum holes: 7 */
        /* last cacheline: 32 bytes */
};

We could pack the struct

$ pahole -C ugly_struct2 demos/small.o                                   
struct ugly_struct2 {                                                                                                             
        char                       padding[1];           /*     0     1 */         
        hash_state                 state;                /*     1   856 */                                                        
                                                                 
        /* size: 857, cachelines: 14, members: 2 */                                                                               
        /* last cacheline: 25 bytes */                 
} __attribute__((__packed__));      

but this would then also lead to

warning: taking address of packed member 'state' of class or structure 'ugly_struct2' may result in an unaligned pointer value [-Waddress-of-packed-member]

There was DCIT/perl-CryptX#95 which lead to introducing the dynamic aligning of buffers, so even though we could in theory even handle completely unaligned accesses we should maybe only test multiples of 8 which should in turn not produce UB. Right?

[MarekKnapek]

Unfortunately this won't work to demo the fixup of misaligned buffers

I do not understand. The ugly_struct will definitely demo the misalignment of the state variables. If the state needs to be aligned more strictly than any other fundamental type. Such as to 16 bytes (for SHA-1 and SHA-256), and largest fundamental type is ulong64 which is not enough to be aligned to 16 bytes.

The padding[1] will shift the structure by 8 bytes, or maybe by 4 bytes on 32bi platforms. There was never an intend to mis-align the structure by 1 byte by me. As this is impossible without invoking undefined behavior.

If you are talking about aligning buffers instead of state, this is different. I never intended to align the data buffers and always used the unaligned access in my intrinsics (except briefly, but I force pushed the change back).

The alignment requirement seems to bring more troubles than benefit. There is always an option to not require any alignment (besides the fundamental C one) and use different intrinsics to read from and write to the state variables.