avoid null pointer dereference when attrSet->pos is null by manveru · Pull Request #4895 · NixOS/nix

manveru · 2021-06-08T08:37:31Z

This may be a fix for #4893

I added a small test that just checks for the segfault, given that builtins.tryEval cannot catch this kind of error.

To recap, here's the behavior before:

❯ nix eval --expr 'builtins.getAttr "bar" (builtins.listToAttrs [ { name = "foo"; value = "bar"; } ])'
[1]    1731608 segmentation fault (core dumped)  nix eval --expr

And after the fix:

❯ ./result/bin/nix eval --expr 'builtins.getAttr "bar" (builtins.listToAttrs [ { name = "foo"; value = "bar"; } ])'
error: attribute 'bar' missing for call to 'getAttr'

       at «string»:1:1:

            1| builtins.getAttr "bar" (builtins.listToAttrs [ { name = "foo"; value = "bar"; } ])
             | ^

thufschmitt

Thanks for the fix!

I have a couple of comments to clean this a slight bit, but overall 👍🏻

thufschmitt · 2021-06-08T12:34:29Z

src/libexpr/primops.cc


-        Pos aPos = *attrSet->pos;
-        if (aPos == noPos) {
+        if (attrSet->pos == nullptr) {


This might be factored out a slight bit I guess. Something along the lines of:

if (attrSet->pos && *attrSet->pos != noPos) { // Throw with the right location and the stack trace } else { // Throw with the parent location }

Does this look OK to you now?

thufschmitt · 2021-06-08T12:42:13Z

tests/list-to-attrs-nullptr.sh

@@ -0,0 +1,7 @@
+#!/usr/bin/env bash


Rather than adding a brand new toplevel test (which does cost a bit as initializing a test is slightly expensive), I think you can

Add this to tests/lang/eval-fail-list-to-attrs.nix

Change https://github.com/NixOS/nix/blob/master/tests/lang.sh#L35 to if ! expect 1 nix-instantiate --eval lang/$i.nix; then (I checked, all the other tests do return 1 as they should)

Also: Can you add a reference to the issue in the test file, so that we know why it’s here?

thufschmitt

Looks good now!

manveru · 2021-06-08T15:51:43Z

Thanks for all the guidance :)

roberth · 2021-06-22T08:14:56Z

I've run nixUnstable with this patch for some days now and it caused a significant improvement in stability.

cc @edolstra

domenkozar · 2021-06-24T16:31:03Z

I plan to merge this in a few days unless someone raises a concern.

edolstra · 2021-06-25T14:50:52Z

@domenkozar I want to have a look at it first.

roberth · 2021-06-29T18:35:18Z

The majority of other sites where pos is dereferenced do not check for null either.
So while this PR does make pos handling a bit more robust, it doesn't seem to be a complete fix.
Part of the problem is that the codebase seems to have two representations of "no position": null or &noPos. The latter is more robust; more than std::optional<Pos> even, because it doesn't ever need to throw or crash. It would seem best to remove all null initializations of pos with an assert(pos) in the appropriate Attr constructor. However, I didn't spot any such initializations in a quick sample. Perhaps the problem is that some Bindings aren't initialized correctly?

Meanwhile, it doesn't hurt to merge this.

thufschmitt · 2021-06-30T08:01:47Z

Part of the problem is that the codebase seems to have two representations of "no position": null or &noPos. The latter is more robust; more than std::optional<Pos> even, because it doesn't ever need to throw or crash.

Couldn’t we make pos a ref (assuming that ref has no runtime overhead)? That way we can statically enforce that it can’t be null and we sidestep this issue

It would seem best to remove all null initializations of pos with an assert(pos) in the appropriate Attr constructor. However, I didn't spot any such initializations in a quick sample.

IIRC in the current case, we explicitly pass a pos argument to the Attr constructor, but this argument happens to be null

Ma27

This fixes a fairly weird problem I had when forgetting to pass version (along with pname) to buildRustPackage in an overlay evaluated by a flake.

roberth · 2021-07-09T13:15:47Z

@edolstra This change is an improvement. Could you merge it?
It may not be the final fix, but the improved stability in the meanwhile will be greatly appreciated.

manveru · 2021-08-26T12:37:13Z

Did another rebase and squashed the commits to make this hopefully a bit more attractive to merge :)

Ma27 · 2021-08-26T12:41:18Z

@edolstra would be awesome to see this merged :)

This fixes a class of crashes and introduces ptr<T> to make the code robust against this failure mode going forward. Thanks regnat for the idea of a ref<T> without overhead! Closes NixOS#4895 Closes NixOS#4893 Closes NixOS#5127 Closes NixOS#5113

roberth · 2021-08-29T19:14:55Z

There's #5192 now, which builds on the ideas from this PR and is more thorough.

manveru · 2021-08-29T19:19:05Z

Awesome, that looks way better!
I'll close this PR then.

manveru force-pushed the work-around-missing-attrSet-pos branch 2 times, most recently from e833200 to f93d30d Compare June 8, 2021 09:03

thufschmitt suggested changes Jun 8, 2021

View reviewed changes

thufschmitt approved these changes Jun 8, 2021

View reviewed changes

jktr mentioned this pull request Jun 9, 2021

segfault in primops' getattr #4852

Closed

domenkozar requested a review from edolstra June 10, 2021 14:11

roberth approved these changes Jun 17, 2021

View reviewed changes

This was referenced Jun 24, 2021

Fix crash in garbage collector #4941

Closed

Fix gc crash #4944

Merged

Ma27 approved these changes Jun 30, 2021

View reviewed changes

handle attrSet->pos being null after listToAttrs

11dbec4

manveru force-pushed the work-around-missing-attrSet-pos branch from 1e41462 to 11dbec4 Compare August 26, 2021 12:36

Ma27 mentioned this pull request Aug 26, 2021

Mitigate a segmentation fault when a derivation has no name attribute #5127

Closed

roberth mentioned this pull request Aug 29, 2021

Force all Pos* to be non-null #5192

Merged

manveru closed this Aug 29, 2021

Uh oh!

Conversation

manveru commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

thufschmitt Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

manveru Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

thufschmitt Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

manveru commented Jun 8, 2021

Uh oh!

roberth commented Jun 22, 2021

Uh oh!

domenkozar commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edolstra commented Jun 25, 2021

Uh oh!

roberth commented Jun 29, 2021

Uh oh!

thufschmitt commented Jun 30, 2021

Uh oh!

Ma27 left a comment

Choose a reason for hiding this comment

Uh oh!

roberth commented Jul 9, 2021

Uh oh!

manveru commented Aug 26, 2021

Uh oh!

Ma27 commented Aug 26, 2021

Uh oh!

roberth commented Aug 29, 2021

Uh oh!

manveru commented Aug 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

manveru commented Jun 8, 2021 •

edited

Loading

domenkozar commented Jun 24, 2021 •

edited

Loading