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

fix segfault and type changing weirdness caused by use-after-free bugs, caused by lack of GC roots #3609

Open
wants to merge 1 commit into
base: master
from

Conversation

@cleverca22
Copy link
Contributor

cleverca22 commented May 22, 2020

some examples of the bug:

Loading '.'...
Added 6 variables.

nix-repl> haskellPackages
trace: Using IOHK default nixpkgs
trace: WARNING: `cleanGit` called on /home/clever/iohk/cardano-base without a `name`. Consider adding `name = "cardano-base;"`
trace: Using index-state: 2020-04-01T00:00:00Z for source
{ cardano-binary = { ... }; cardano-binary-test = { ... }; cardano-crypto-class = { ... }; cardano-slotting = { ... }; recurseForDerivations = true; }



nix-repl> haskellPackages
«unknown»

the Value got entirely overwritten by junk and this is a clear use-after-free

nix-repl> haskellPackages
trace: Using IOHK default nixpkgs
trace: WARNING: `cleanGit` called on /home/clever/iohk/cardano-base without a `name`. Consider adding `name = "cardano-base;"`
trace: Using index-state: 2020-04-01T00:00:00Z for source
{ cardano-binary = { ... }; cardano-binary-test = { ... }; cardano-crypto-class = { ... }; cardano-slotting = { ... }; recurseForDerivations = true; }

nix-repl> haskellPackages
"0.0.2"

the Value got overwritten by a different Value, due to the alignment matching up, it appears to just change contents magically

nix-repl> haskellPackages.cardano-crypto-class.components
trace: Using IOHK default nixpkgs
trace: WARNING: `cleanGit` called on /home/clever/iohk/cardano-base without a `name`. Consider adding `name = "cardano-base;"`
trace: Using index-state: 2020-04-01T00:00:00Z for source
[2 copied (0.0 MiB), 0.0 MiB DL]«derivation /nix/store/lm18qks8s4yamyzwvsniqz2i41zsd2n5-cardano-crypto-class-2.0.0-all.drv»; benchmarks = { ... }; exes = { ... }; foreignlibs = { ... }; library = «derivation /nix/store/zdbfhibz8ms747m5sr6y5plr30bdvxl3-cardano-crypto-class-2.0.0-lib-cardano-crypto-class.drv»; recurseForDerivations = true; sublibs = { ... }; tests = { ... }; }

nix-repl> haskellPackages.cardano-crypto-class.components.tests
error: attribute 'components' missing, at (string):1:1
[2 copied (0.0 MiB), 0.0 MiB DL]
nix-repl> haskellPackages.cardano-crypto-class
{ _type = "option-type"; check = «lambda @ /nix/store/vi2klbspnnpvkyp6qygj14ff01npqbqi-source/lib/types.nix:346:15»; description = "null or string"; functor = { ... }; getSubModules = null; getSubOptions = «lambda @ /nix/store/vi2klbspnnpvkyp6qygj14ff01npqbqi-source/lib/types.nix:70:23»; merge = «lambda @ /nix/store/vi2klbspnnpvkyp6qygj14ff01npqbqi-source/lib/types.nix:347:15»; name = "nullOr"; substSubModules = «lambda @ /nix/store/vi2klbspnnpvkyp6qygj14ff01npqbqi-source/lib/types.nix:355:25»; typeMerge = «lambda @ /nix/store/vi2klbspnnpvkyp6qygj14ff01npqbqi-source/lib/types.nix:23:25»; }

a regular set magically turned into a module option, due to the Value getting overwritten again

i believe all of these are caused by the pointers within the NixRepl class not being counted as GC roots (due to them not being allocated via boehmgc)
so when a GC does run, it deletes things that NixRepl had pointers to, and then the next time you eval something, your using invalid pointers

…s, caused by lack of GC roots
@edolstra
Copy link
Member

edolstra commented May 22, 2020

Shouldn't this already be addressed by NixRepl inheriting from gc?

@edolstra
Copy link
Member

edolstra commented May 22, 2020

There's also the RootValue type in value.hh. Seems better than manual calls to GC_add_roots / GC_remove_roots.

@cleverca22
Copy link
Contributor Author

cleverca22 commented May 23, 2020

ah, i see, class gc overrides new and forces all c++ objects to come from the boehm heap, yeah, i would expect that to fix the problem my changes fix

the annoying thing, is how random the bug is, and how the changes in this PR did make the problem go away, but maybe it just tweaked the alignment of the stars enough for it to corrupt something else....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.