On Thu, 2017-10-26 at 12:47 +0200, Lukas Slebodnik wrote: > 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. > >
Thanks for the great and thorough answer mate. I'm not feeling so good this week, but I'll read this through and write up a patch early next week for this. Thanks again! -- Sincerely, William Brown Software Engineer Red Hat, Australia/Brisbane
signature.asc
Description: This is a digitally signed message part
_______________________________________________ 389-devel mailing list -- 389-devel@lists.fedoraproject.org To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org