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
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ 389-devel mailing list -- 389-devel@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to 389-devel-leave@xxxxxxxxxxxxxxxxxxxxxxx
