Skip to content

assign{Ips,Macs}: Fix integer overflow#1

Open
GeoffreyFrogeye wants to merge 1 commit intooddlama:mainfrom
GeoffreyFrogeye:push-rylkmzxxpxyw
Open

assign{Ips,Macs}: Fix integer overflow#1
GeoffreyFrogeye wants to merge 1 commit intooddlama:mainfrom
GeoffreyFrogeye:push-rylkmzxxpxyw

Conversation

@GeoffreyFrogeye
Copy link
Copy Markdown

Half of SHA256 sums truncated to 16 chars actually go over 2^63, which is beyond the range for hexToDec.
This fixes the integer overflow error in Lix 2.91+ and Nix 2.25+, as well as undefined behaviour for versions below

Half of SHA256 sums truncated to 16 chars actually go over 2^63,
which is beyond the range for hexToDec.
This fixes the integer overflow error in Lix 2.91+ and Nix 2.25+,
as well as undefined behaviour for versions below
@PatrickDaG
Copy link
Copy Markdown
Collaborator

Hey. Thanks a lot. I also ran into this problem a few weeks ago, when trying to switch to lix. While your solution does fix the problem for this project, as far as I know, it is only solving the one case where it's most likely to happen, and not the underlying problem. One can still call the function exposed by this library and encounter the overflow.

I think the best solution would be to get nix/lix to implement either the standard shift operators, or at least some kind of unsafe mul.

I have a lix fork that I am using currently, which implements these over at https://forge.lel.lol/patrick/lix/, I just haven't had the time to make a pull requests upstream. You can check out the lix-compat branch to see them in action.

@GeoffreyFrogeye
Copy link
Copy Markdown
Author

GeoffreyFrogeye commented Apr 8, 2025

Right, I realize now this fix only works for assignIps with IPv4, for IPv6 and assignMacs it straight out doesn't seem to work at all.

Really cool that you have a proper solution already in the works. Even with adding a feature to Lix it saves code overall. Makes the assign* code cleaner too, I must admit I only managed to come up with this fix for my use case because hexToDec was properly commented, noting its domain.

Feel free to close this PR, unless you think this could still be used as a stopgap.

@spacekitteh
Copy link
Copy Markdown
Contributor

Right, I realize now this fix only works for assignIps with IPv4, for IPv6 and assignMacs it straight out doesn't seem to work at all.

Damn, I exclusively use IPv6 :(

@spacekitteh
Copy link
Copy Markdown
Contributor

Relevant: NixOS/nix#13001

@GeoffreyFrogeye
Copy link
Copy Markdown
Author

Also relevant: https://gerrit.lix.systems/c/lix/+/2975 . Glad you're using the same function names :)

@PatrickDaG
Copy link
Copy Markdown
Collaborator

I've implemented a shift operation that should work with current nix here: ddc2349. I think we'll use that for the time being until we know what's the status with the new builtins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants