Skip to content

Change CSipHasher's count variable to uint8_t #19931

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 14, 2020
Merged

Change CSipHasher's count variable to uint8_t #19931

merged 1 commit into from
Sep 14, 2020

Conversation

sipa
Copy link
Member

@sipa sipa commented Sep 10, 2020 •
edited
Loading

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.

@sipa sipa mentioned this pull request Sep 10, 2020
Closed
@laanwj
Copy link
Member

laanwj commented Sep 10, 2020 •
edited
Loading

Very nice and straightforward solution.
ACK e3318f4

@practicalswift
Copy link
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 :)

@sipa sipa force-pushed the 202009_siphash_sillyness branch from e3318f4 to 812037c Compare September 10, 2020 16:05
@elichai
Copy link
Contributor

elichai commented Sep 10, 2020

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

@sipa
Copy link
Member Author

sipa commented Sep 10, 2020

@practicalswift Fixed.

@sipa
Copy link
Member Author

sipa commented Sep 10, 2020

@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.

@practicalswift
Copy link
Contributor

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

ACK 812037c

@laanwj
Copy link
Member

laanwj commented Sep 11, 2020 •
edited
Loading

@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 😄 (oh, crap, it might even be slower because the &0xff sometimes needs to be done explicitly)

anyhow re-ACK 812037c

theStack
theStack approved these changes Sep 12, 2020
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 812037c

@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?

@fanquake fanquake merged commit 06dbbe7 into bitcoin:master Sep 14, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 15, 2020
@fanquake
Merge bitcoin#19931: Change CSipHasher's count variable to uint8_t
2db3208 
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), 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 bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 17, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#19931: Change CSipHasher's count variable to uint8_t
0b9240c 
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), 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 bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 24, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#19931: Change CSipHasher's count variable to uint8_t
25f21c1 
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), 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 bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 5, 2021
@sipa @PiRK
Change CSipHasher's count variable to uint8_t
1df9953 
Summary:
> 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.

This is a backport of [[bitcoin/bitcoin#19931 | core#19931]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10241
kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 12, 2021
@fanquake @kwvg
Merge bitcoin#19931: Change CSipHasher's count variable to uint8_t
c30d67d 
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), 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 bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
@barton2526 barton2526 mentioned this pull request Oct 21, 2021
Merged
PiRK pushed a commit to PiRK/lotusd that referenced this pull request Aug 16, 2022
@sipa @PiRK
Change CSipHasher's count variable to uint8_t
a6f6aa5 
Summary:
> 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.

This is a backport of [[bitcoin/bitcoin#19931 | core#19931]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10241
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Signed integer overflow when SipHasher processes inputs >= 2 GB
6 participants
@sipa @laanwj @practicalswift @elichai @theStack @fanquake