Avoid accessing uninitialized settings in own init #11117
Conversation
The default value for the setting was evaluated by calling a method on the object _being currently constructed_, so we were using it before all fields were initialized. This has been fixed by making the called method static, and not using the previously used fields at all. But functionality hasn't changed! The fields were usually always zero (by chance?) anyway, meaning the conditional path was always taken. Thus the current logic has been kept, the code simplified, and UB removed. This was found with the helper of UBSan.
|
CC @roberth and @fricklerhandwerk who have been tracking down this big connundrum |
There was a problem hiding this comment.
I think we should instead fix this by changing the field order so those other two settings are initialized first.
Is that ugly has hell? Yes absolutely. But I think that is more correct for now, and a better solution can come from the settings system redoing.
| @@ -69,11 +69,9 @@ Strings EvalSettings::getDefaultNixPath() const | |||
| } | |||
| }; | |||
|
|
|||
| if (!restrictEval && !pureEval) { | |||
There was a problem hiding this comment.
I wasn't aware how problematic this conditional was, but in my mental model it was always initialized to those three elements anyway, because we looked at it from the perspective of the initialization of the settings object anyway, of which we were aware that it was not in a properly loaded state yet. But yeah, it's worse; not even properly initialized, but in effect it doesn't matter which it is.
off-topic: I'd much prefer a good ol' infinite recursion over this stateful mess :D
There was a problem hiding this comment.
Indeed, an option would be to make all settings (virtual) methods, such that they can compute.
There was a problem hiding this comment.
That'd be alright, and a call depth limit could replace tBlackhole in this comparison.
|
@Ericson2314 But that would change the logic, and I'm not sure we even want that. It has caused no issues until now. Why would you change the default depending on whether restrict-eval or pure-eval are enabled? Also, I'm not familiar with how settings are read: |
IIUC they'd only be in an initialized state; not a loaded / parsed state, so it'd just give a false sense of correctness. |
|
@edolstra the change here indeed doesn't change behavior, and we'll add tests with #11079 to ensure the right things end up happening. @Ericson2314 I suggest we merge small increments now, because redoing the settings system is a big chunk of work. |
The default value for the setting was evaluated by calling a method on the object being currently constructed, so we were using it before all fields were initialized.
This has been fixed by making the called method static, and not using the previously used fields at all.
But functionality hasn't changed!
The fields were usually always zero (by chance?) anyway, meaning the conditional path was always taken.
Thus the current logic has been kept, the code simplified, and UB removed.
This was found with the helper of UBSan.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.