Change CSipHasher's count variable to uint8_t #19931

pull sipa wants to merge 1 commits into bitcoin:master from sipa:202009_siphash_sillyness changing 2 files +2 −2
  1. sipa commented at 7:31 AM on September 10, 2020: member

    SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the paper), but only the low 8 bits of the length matter. Because of that we should use an unsigned type to track the length (as any signed type could overflow, which is UB). uint8_t is sufficient, however.

    Fixes #19930.

  2. sipa cross-referenced this on Sep 10, 2020 from issue Signed integer overflow when SipHasher processes inputs >= 2 GB by practicalswift
  3. fanquake added the label Refactoring on Sep 10, 2020
  4. laanwj commented at 12:26 PM on September 10, 2020: member

    Very nice and straightforward solution. ACK e3318f4b06a5dfc3bf5a2a1592ac16ff7b306560

  5. practicalswift commented at 2:28 PM on September 10, 2020: contributor

    @sipa I'm afraid the fix is incorrect: to get rid of the signed integer overflow you'll have to change also the local variable c in CSipHasher::Write to uint8_t :)

  6. Change CSipHasher's count variable to uint8_t 812037cb80
  7. sipa force-pushed on Sep 10, 2020
  8. elichai commented at 7:16 PM on September 10, 2020: contributor

    FWIW this is also fixed in #18014 but I like the decrese in the size of CSipHasher :) utACK 812037cb80f72096738cf2b0c15b39536c6c1e24

  9. sipa commented at 9:23 PM on September 10, 2020: member
  10. sipa commented at 9:23 PM on September 10, 2020: member

    @elichai It's not actually a decrease in size, as its alignment is 8 bytes (at least on 64 bit platforms), so the size is 24 bytes before and after.

  11. practicalswift commented at 6:04 AM on September 11, 2020: contributor

    Thanks for the swift (and practical!) fix! :)

    ACK 812037cb80f72096738cf2b0c15b39536c6c1e24

  12. laanwj commented at 2:13 PM on September 11, 2020: member

    @elichai It's not actually a decrease in size, as its alignment is 8 bytes (at least on 64 bit platforms), so the size is 24 bytes before and after.

    It's not like we store millions of SipHashers for this to matter anyway. Nor does it matter for performance on any supported architecture :smile: (oh, crap, it might even be slower because the &0xff sometimes needs to be done explicitly)

    anyhow re-ACK 812037cb80f72096738cf2b0c15b39536c6c1e24

  13. theStack approved
  14. theStack commented at 3:05 PM on September 12, 2020: contributor

    ACK 812037cb80f72096738cf2b0c15b39536c6c1e24

    @elichai It's not actually a decrease in size, as its alignment is 8 bytes (at least on 64 bit platforms), so the size is 24 bytes before and after.

    I guess you meant 6*8 = 48 bytes?

  15. fanquake merged this on Sep 14, 2020
  16. fanquake closed this on Sep 14, 2020

  17. sidhujag referenced this in commit 2db3208004 on Sep 15, 2020
  18. PastaPastaPasta referenced this in commit 0b9240c37a on Sep 17, 2021
  19. PastaPastaPasta referenced this in commit 25f21c103f on Sep 24, 2021
  20. Fabcien referenced this in commit 1df995312b on Oct 5, 2021
  21. kwvg referenced this in commit c30d67d14b on Oct 12, 2021
  22. barton2526 cross-referenced this on Oct 21, 2021 from issue refactor, build: Upstream fixes for the /crypto files. Implement Keccak and SHA3 by barton2526
  23. PiRK referenced this in commit a6f6aa5c29 on Aug 16, 2022
  24. bitcoin locked this on Aug 16, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:53 UTC