@lslebodn @vashirov Would really appreciate you both looking at this - to check my solution matches expectation and also that it works on ppc64 which had the issue,

Thanks!

+    size_t input_sz = src_sz / sizeof(uint64_t);
+    /* Account for non-uint64_t alligned input */
+    if (src_sz % sizeof(uint64_t) > 0) {
+        input_sz += 1;
+    }

IMHO you can always add +1 to input_sz But that's micro optimisation :-)
and I'm fine with current version.

I think there still can be a problem with following code:

    118     uint64_t t = 0;
    119     uint8_t *pt = (uint8_t *)&t;
    120     uint8_t *m = (uint8_t *)in;
    121 
    122     switch (src_sz) {
    123     case 7:
    124         pt[6] = m[6]; /* FALLTHRU */
    125     case 6:
    126         pt[5] = m[5]; /* FALLTHRU */
    127     case 5:
    128         pt[4] = m[4]; /* FALLTHRU */
    129     case 4:
    130         *((uint32_t *)&pt[0]) = *((uint32_t *)&m[0]);

I am not sure whether ((intptr_t)m % sizeof(uint32_t) == 0)

Test no longer crashes, but still fails:

[==========] Running 1 test(s).
[ RUN      ] test_siphash
[  ERROR   ] --- hashout == test_b
[   LINE   ] --- src/libsds/test/test_sds_csiphash.c:44: error: Failure!
[  FAILED  ] test_siphash

BTW if you would like to get rid of dynamic allocation you can try following change. I didn't test taht

diff --git a/src/libsds/external/csiphash/csiphash.c b/src/libsds/external/csiphash/csiphash.c
index 0089c82f7..07406f602 100644
--- a/src/libsds/external/csiphash/csiphash.c
+++ b/src/libsds/external/csiphash/csiphash.c
@@ -79,7 +79,8 @@ sds_siphash13(const void *src, size_t src_sz, const char key[16])
     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;
+    const uint8_t *in = (uint8_t *)src;
+    uint64_t temp;

     uint64_t v0 = k0 ^ 0x736f6d6570736575ULL;
     uint64_t v1 = k1 ^ 0x646f72616e646f6dULL;
@@ -87,8 +88,9 @@ sds_siphash13(const void *src, size_t src_sz, const char key[16])
     uint64_t v3 = k1 ^ 0x7465646279746573ULL;

     while (src_sz >= 8) {
-        uint64_t mi = _le64toh(*in);
-        in += 1;
+        memcpy(&temp, in, 8);
+        uint64_t mi = _le64toh(temp);
+        in += 8;
         src_sz -= 8;
         v3 ^= mi;
         // cround

@vashirov We might get better error reporting with different cmocka assertions:

diff --git a/src/libsds/test/test_sds_csiphash.c b/src/libsds/test/test_sds_csiphash.c
index cdb6b7f46..1a50401ee 100644
--- a/src/libsds/test/test_sds_csiphash.c
+++ b/src/libsds/test/test_sds_csiphash.c
@@ -37,11 +37,11 @@ test_siphash(void **state __attribute__((unused)))
     // Initial simple test
     value = htole64(5);
     hashout = sds_siphash13(&value, sizeof(uint64_t), key);
-    assert_true(hashout == test_a);
+    assert_int_equal(hashout, test_a);

     char *test = "abc";
     hashout = sds_siphash13(test, 4, key);
-    assert_true(hashout == test_b);
+    assert_int_equal(hashout, test_b);
 }

Perhaps this is an endianness issues with the memcpy?

BTW if you would like to get rid of dynamic allocation you can try following change. I didn't test taht
diff --git a/src/libsds/external/csiphash/csiphash.c b/src/libsds/external/csiphash/csiphash.c
index 0089c82..07406f602 100644
--- a/src/libsds/external/csiphash/csiphash.c
+++ b/src/libsds/external/csiphash/csiphash.c
@@ -79,7 +79,8 @@ sds_siphash13(const void src, size_t src_sz, const char key[16])
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;
+ const uint8_t in = (uint8_t *)src;
+ uint64_t temp;

uint64_t v0 = k0 ^ 0x736f6d6570736575ULL;
uint64_t v1 = k1 ^ 0x646f72616e646f6dULL;
@@ -87,8 +88,9 @@ sds_siphash13(const void *src, size_t src_sz, const char key[16])
uint64_t v3 = k1 ^ 0x7465646279746573ULL;

while (src_sz >= 8) {
- uint64_t mi = _le64toh(*in);
- in += 1;
+ memcpy(&temp, in, 8);
+ uint64_t mi = _le64toh(temp);
+ in += 8;
src_sz -= 8;
v3 ^= mi;
// cround

But we don't know how large the input is, so I think this may have a memcpy issues if in is not large enough (stack over-read).

@vashirov We might get better error reporting with different cmocka assertions:
diff --git a/src/libsds/test/test_sds_csiphash.c b/src/libsds/test/test_sds_csiphash.c
index cdb6b7f..1a50401ee 100644
--- a/src/libsds/test/test_sds_csiphash.c
+++ b/src/libsds/test/test_sds_csiphash.c
@@ -37,11 +37,11 @@ test_siphash(void **state attribute((unused)))
// Initial simple test
value = htole64(5);
hashout = sds_siphash13(&value, sizeof(uint64_t), key);
- assert_true(hashout == test_a);
+ assert_int_equal(hashout, test_a);

char *test = "abc";
hashout = sds_siphash13(test, 4, key);
- assert_true(hashout == test_b);
+ assert_int_equal(hashout, test_b);
}

Done, added to the patch :)

• size_t input_sz = src_sz / sizeof(uint64_t);
• / Account for non-uint64_t alligned input /
• if (src_sz % sizeof(uint64_t) > 0) {
• input_sz += 1;
• }

IMHO you can always add +1 to input_sz But that's micro optimisation :-)

Done, it's simple :)

and I'm fine with current version.
I think there still can be a problem with following code:
118 uint64_t t = 0;
119 uint8_t pt = (uint8_t )&t;
120 uint8_t m = (uint8_t )in;
121
122 switch (src_sz) {
123 case 7:
124 pt[6] = m[6]; / FALLTHRU /
125 case 6:
126 pt[5] = m[5]; / FALLTHRU /
127 case 5:
128 pt[4] = m[4]; / FALLTHRU /
129 case 4:
130 ((uint32_t )&pt[0]) = ((uint32_t )&m[0]);

I am not sure whether ((intptr_t)m % sizeof(uint32_t) == 0)

I think at this point it shouldn't matter, because it's just doing 32bit reads of the values, and pt is from &t which is a uint64_t, so that will be valid, and &m is from *in, which we just fixed to be aligned. So in both, this should be okay now.

    while (src_sz >= 8) {
        uint64_t mi = _le64toh(*in);
        in += 1;
        src_sz -= 8;
        v3 ^= mi;
        // cround
        cROUND(v0, v1, v2, v3);
        v0 ^= mi;
    }

    uint64_t t = 0;
    uint8_t *pt = (uint8_t *)&t;
    uint8_t *m = (uint8_t *)in;

Actually, I think the issue is here. We correctly copy in everything with le64toh for mi in uint64_t groups, but when we have a src without an input of size 8, we will hit uint8_t m = (uint8_t )in; which does not do the same le64toh. So this could be the issue.

[   LINE   ] --- src/libsds/test/test_sds_csiphash.c:44: error: Failure!

^^-- Yep, that's the failure. On an aligned source we pass, but line 44 is an input of 24 bytes, so we'll skip that loop and go to the uint8_t m = (uint8_t )in; line, which is not correctly using endianness controls. As a result we get the failure.

The first test will be src_sz = 8, and the second is src_sz = 3, so that's why this happens. I'm still not sure of the fix though ....

This is what I have now, but it's still not solving the issues from the line 44 test.

So a build/cmocka test on copr with ppc64le works, so this is definitely an endianness issue with non-multiple of 8 sized input data.

So a build/cmocka test on copr with ppc64le works, so this is definitely an endianness issue with non-multiple of 8 sized input data.

That's possible :-)

BTW it might be good idea to extend test suite and test string with length from 8 till 16. Unit tests are fast and it will cover all cases with endian issues.

Yes, this is a good idea. But you are also still assuming that I know how to solve this issue in a meaningful way :) I'll update the test cases for a start,

Okay, @vashirov here is a new patch which uses a different strategy to do the memcpy for excess bytes. I'll stay online for your morning to work through it with you. Thanks to @lslebodn for his advice, I added extra tests too and they all pass on LE x86_64

seems to work on Solaris11.3 Sparc:

[==========] Running 1 test(s).
[ RUN      ] test_siphash
[       OK ] test_siphash
[==========] 1 test(s) run.
[  PASSED  ] 1 test(s).

Great! @cgrzemba thanks for testing! @vashirov is about to check this on a bigendian system to be sure the fix is correct.

Thanks for your patience with this,

make  check-TESTS
make[2]: Entering directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
make[3]: Entering directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
PASS: test_slapd
PASS: test_libsds
PASS: test_nuncstans
PASS: test_nuncstans_stress_small
make[4]: Entering directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
make  all-am
make[5]: Entering directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
make[5]: Nothing to be done for `all-am'.
make[5]: Leaving directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
make[4]: Leaving directory `/root/rpmbuild/BUILD/389-ds-base-1.3.7.5'
============================================================================
Testsuite summary for 389-ds-base 1.3.7.5
============================================================================
# TOTAL: 4
# PASS:  4
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

All tests pass on ppc64 :)

LGTM, ack

commit 7514464
To ssh://git@pagure.io/389-ds-base.git
e3d4a52..7514464 master -> master

commit 92c56e9
To ssh://git@pagure.io/389-ds-base.git
01530f4..92c56e9 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2483

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.