Skip to content
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.

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

Use portable C++ Pseudorandom number generator #10541

Open
Ericson2314 opened this issue Apr 17, 2024 · 10 comments
Open

Use portable C++ Pseudorandom number generator #10541

Ericson2314 opened this issue Apr 17, 2024 · 10 comments
Labels
good first issue Quick win for first-time contributors portability Supporting more platforms windows

Comments

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 17, 2024

Avoid calls to rand / srand / random / srandom and other things which are not portable to Windows

Originally posted by @edolstra in #8901 (comment)

@Ericson2314 Ericson2314 added windows good first issue Quick win for first-time contributors labels Apr 17, 2024
@Ericson2314 Ericson2314 added the portability Supporting more platforms label Apr 17, 2024
@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-on-windows/1113/109

@RCoeurjoly
Copy link
Contributor

I see that random() is called in two places:

src/libstore/unix/gc.cc:44: Path tempLink = fmt("%1%.tmp-%2%-%3%", link, getpid(), random());
src/libstore/unix/optimise-store.cc:228: Path tempLink = fmt("%1%/.tmp-link-%2%-%3%", realStoreDir, getpid(), random());

Should we create a PRNG before every call?
Or create a PRNG in initNix that somehow gets passed to the functions that need it?

@Ericson2314
Copy link
Member Author

@RCoeurjoly good question! When in doubt, it is better to avoid global state.

I think for the gc one, it would be good to put the PRNG state in IndirectRootStore, and since LocalStore is a subclass, the optimise-store one can use that too.

Thanks for looking into this!

@dottharun
Copy link
Contributor

I see another random() call in - src/libstore/unix/build/derivation-goal.cc:826 too

@Ericson2314
Copy link
Member Author

That can also use a per-store PRNG

@RCoeurjoly
Copy link
Contributor

The problem is that random() in gc.cc:44 is called by makeSymlink(const Path & link, const Path & target), a free function, so there is no way to access a per-store PRNG.

I was thinking of the following:

Initialize the PRNG in initNix, as it is done now with srandom().
Use a class with static members.

Use this class wherever is needed.

Thoughts?

@dottharun
Copy link
Contributor

IndirectRootStore Mix-in seems to have a clear purpose of providing its two methods

  • do we have to use it for PRNG just for its structure?
  • maybe some other class in libutil/ as seeding needs to done only once?

@Ericson2314
Copy link
Member Author

@RCoeurjoly After #10556 it will not be a free function

@dottharun And the PRNG will be for one the IndirectRootStore methods; the indirect root symlinks not colliding in the directory is part of the spec! :)

@Ericson2314
Copy link
Member Author

Ericson2314 commented Apr 22, 2024

After #10556 it will not be a free function

If anyone wants to take this issue, they should feel free to do just redo that part of #10556 in their PR. That is better than waiting for my PR.

@Ericson2314
Copy link
Member Author

#10556 is now merged, so this should be a bit easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Quick win for first-time contributors portability Supporting more platforms windows
Projects
None yet
Development

No branches or pull requests

4 participants