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(libexpr/eval-inline): get rid of references to nullptr env #11881

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xokdvium
Copy link
Contributor

Motivation

When diagnosing infinite recursion references to nullptr Env can be formed. This happens only with ExprBlackHole is evaluated, which always leads to InfiniteRecursionError.

UBSAN log for one such case:

../src/libexpr/eval-inline.hh:94:31: runtime error: reference binding to null pointer of type 'Env'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/libexpr/eval-inline.hh:94:31 in

Context

#10969

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

When diagnosing infinite recursion references to nullptr `Env` can be formed.
This happens only with `ExprBlackHole` is evaluated, which always leads to
`InfiniteRecursionError`.

UBSAN log for one such case:

```
../src/libexpr/eval-inline.hh:94:31: runtime error: reference binding to null pointer of type 'Env'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/libexpr/eval-inline.hh:94:31 in
```
@xokdvium
Copy link
Contributor Author

xokdvium commented Nov 14, 2024

// TBD: Maybe this case be handled directly without an idirection and a dummy
// environment (which isn't accesses anyway)?

I'm really not sure that this is the best way. This patch is pretty conservative, but I'm pretty sure that the only case when env is nullptr is for ExprBlackHole, since the assertion assert(env || v.isBlackhole()) holds in tests. I hope someone can shed some light on this.

If I'm correct in the assumption above, we could just move the code that throws InfiniteRecursionError to a member function that does not need an Env parameter and call it directly instead of eval with a dummy parameter. Since this is in the cold path and in the [[inlikely]] branch that throws perf should be of no concern hopefully.

@xokdvium
Copy link
Contributor Author

Fixes #11854

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.

1 participant