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

Ensure enough space in attrset bindings #3238

Merged
merged 2 commits into from Nov 26, 2019

Conversation

@puckipedia
Copy link
Contributor

puckipedia commented Nov 25, 2019

When allocating bindings for attrset when using __overrides, we need to use the capacity of the original attrs, otherwise there's no space for dynamic attributes.

(rec { "${"foo"}" = 5; __overrides = { b = 5; }; }.foo)
@grahamc
Copy link
Member

grahamc commented Nov 25, 2019

Can you provide a test case?

@puckipedia
Copy link
Contributor Author

puckipedia commented Nov 25, 2019

Added.

rec {
"${"foo"}" = "bar";
__overrides = { bar = "qux"; };
}

This comment has been minimized.

@grahamc

grahamc Nov 25, 2019 Member

before this PR:

[grahamc@Petunia:~]$ ls -la $(which nix)
lrwxrwxrwx 1 root root 59 Dec 31  1969 /run/current-system/sw/bin/nix -> /nix/store/dj2ylmvx2c288h51yx8ljg4h6bh5vyla-nix-2.3/bin/nix

[grahamc@Petunia:~]$ nix repl
Welcome to Nix version 2.3. Type :? for help.

nix-repl> rec {
            "${"foo"}" = "bar";
             __overrides = { bar = "qux"; };
          }
nix: src/libexpr/attr-set.hh:54: void nix::Bindings::push_back(const nix::Attr&): Assertion `size_ < capacity_' failed.
zsh: abort (core dumped)  nix repl

after this PR:

[nix-shell:~/projects/github.com/NixOS/nix]$ ./inst/bin/nix repl --experimental-features nix-command
Welcome to Nix version 2.4. Type :? for help.

nix-repl> rec {
                      "${"foo"}" = "bar";
                       __overrides = { bar = "qux"; };
                    }
{ __overrides = { ... }; bar = "qux"; foo = "bar"; }
@edolstra
Copy link
Member

edolstra commented Nov 25, 2019

I don't understand why the capacity of v.attrs matters, since the size should reflect what we're copying from v.attrs:

            for (auto & i : *v.attrs)
                newBnds->push_back(i);
@puckipedia
Copy link
Contributor Author

puckipedia commented Nov 25, 2019

At the point that the reallocation is done, v.attrs only contains non-dynamic attrs, so using the size means that there is no space left for dynamic attrs after __overrides has been processed.

@edolstra edolstra merged commit 872740c into NixOS:master Nov 26, 2019
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

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