On (13/10/17 20:35), William Brown wrote:
>On Wed, 2017-10-11 at 13:36 +0200, Lukas Slebodnik wrote:
>> But following code should work. Please correct me if I am wrong. I didn't test.
>> char *str = strdup("ABCDEFGH12345678");
>> char *key = malloc(16);
>>
>> yes, function sds_siphash13 is not ideal because it rely on properly alligned
>> input data.
>>
>
>We are free to change the signature of the function, it's just that I
>used this from another open source component (thus why it's slightly
>different style wise)
>
Changing signature will not help. The main problem is that implementation of
function is not really portable
>sds_siphash13(const void *src, size_t src_sz, const char key[16])
>{
> const uint64_t *_key = (uint64_t *)key;
^^^^^^^^^^^^^^^
This cast is portable only in case of "((intptr_t)key) % sizeof(uint64_t) == 0"
It is not a problem in intel/amd but it is not portable
> uint64_t k0 = _le64toh(_key[0]);
> uint64_t k1 = _le64toh(_key[1]);
> uint64_t b = (uint64_t)src_sz << 56;
> const uint64_t *in = (uint64_t *)src;
^^^^^^^^^^^^^^^
and the same situation is here
This is usually not the problem is you directly pass pointer returned by
malloc. But it is not the case on many places.
e.g.
here is an usage on function in libsds
sh$ git grep sds_siphash13 src/libsds/
src/libsds/external/csiphash/csiphash.c:sds_siphash13(const void *src, size_t src_sz, const char key[16])
src/libsds/include/sds.h: * sds_siphash13 provides an implementation of the siphash algorithm for use in
src/libsds/include/sds.h:uint64_t sds_siphash13(const void *src, size_t src_sz, const char key[16]);
src/libsds/sds/ht/op.c: uint64_t hashout = sds_siphash13(key, ht_ptr->key_size_fn(key), ht_ptr->hkey);
src/libsds/sds/ht/op.c: uint64_t ex_hashout = sds_siphash13(slot->slot.value->key, ht_ptr->key_size_fn(slot->slot.value->key), ht_ptr->hkey);
src/libsds/sds/ht/op.c: uint64_t hashout = sds_siphash13(key, ht_ptr->key_size_fn(key), ht_ptr->hkey);
src/libsds/sds/ht/op.c: uint64_t hashout = sds_siphash13(key, ht_ptr->key_size_fn(key), ht_ptr->hkey);
src/libsds/test/test_sds_csiphash.c: hashout = sds_siphash13(&value, sizeof(uint64_t), key);
src/libsds/test/test_sds_csiphash.c: hashout = sds_siphash13(test, 4, key);
ht_ptr->hkey is used on many places and it is not aligned to 64 bites.
Here is a definition of structure
typedef struct _sds_ht_instance
{
uint32_t checksum;
char hkey[16];
sds_ht_node *root;
int64_t (*key_cmp_fn)(void *a, void *b);
uint64_t (*key_size_fn)(void *key);
void *(*key_dup_fn)(void *key);
void (*key_free_fn)(void *key);
void *(*value_dup_fn)(void *value);
void (*value_free_fn)(void *value);
} sds_ht_instance;
This structure is always created with malloc. So the beginning of structure is
aligned. "ht_ptr = sds_calloc(sizeof(sds_ht_instance));"
But it is very possible that hkey will not be aligned to 64 bits unless
compiler adds some padding between "checksum" and "hkey".
You might do the trick to change order of members in structure which would
solve problem here. But it still would not solve problem with improperly
aligned input in sds_siphash13.
let say we have following code
int main(void)
{
char *input = strdup("lorem impsum");
char *key = calloc(1, 16);
uint64_t temp;
/* this version would not crash) */
temp = sds_siphash13(input, strlen(input), key);
/* but this version will crash */
temp = sds_siphash13(input + 1, strlen(input +1), key);
return 0;
}
It is really difficult to rely on proper alignment of input.
In theory you could have assert in code which would catch it
also in x86_64 architecture. But it is not nice solution.
The only portable way is to use memcpy to copy data from
array of char to variable/array of uint64_t.
BTW recently I read nice article and it can crash even on x86_64
in case of vector operations. Which compilers try to use as part of
optimisations.
https://pzemtsov.github.io/2016/11/06/bug-story-alignment-on-x86.html
And as you can see in article usage of memcpy is not so bad especially
if compiler uses builtin instead of procedure call.
HTH and sorry for late reply.
LS
_______________________________________________
389-devel mailing list -- 389-devel@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to 389-devel-leave@xxxxxxxxxxxxxxxxxxxxxxx
