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