Skip to content

nix_api_expr: ensure destructors are called for builder/state#14573

Merged
Ericson2314 merged 1 commit intoNixOS:masterfrom
corngood:libexpr-leak
Nov 16, 2025
Merged

nix_api_expr: ensure destructors are called for builder/state#14573
Ericson2314 merged 1 commit intoNixOS:masterfrom
corngood:libexpr-leak

Conversation

@corngood
Copy link
Contributor

I found this because of a test failure on cygwin in nix_api_expr_test.nix_eval_state_lookup_path:

'std::filesystem::__cxx11::filesystem_error'
what(): filesystem error: cannot remove all: Device or resource busy
[...]
[.../my_state/db/db.sqlite]

LocalState was never getting destroyed due to a reference leak. These _free functions use an 'operator delete' which doesn't call the destructor for the type.

Fixes: 309d558

I found this because of a test failure on cygwin in
nix_api_expr_test.nix_eval_state_lookup_path:

 'std::filesystem::__cxx11::filesystem_error'
   what():  filesystem error: cannot remove all: Device or resource busy
   [...]
   [.../my_state/db/db.sqlite]

LocalState was never getting destroyed due to a reference leak.  These
_free functions use an 'operator delete' which doesn't call the
destructor for the type.

Fixes: 309d558
@corngood corngood requested a review from edolstra as a code owner November 15, 2025 19:40
@github-actions github-actions bot added the c api Nix as a C library with a stable interface label Nov 15, 2025
Copy link
Contributor

@xokdvium xokdvium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sucks that we are using the operator new/delete that just allocate raw bytes. I have no clue why it's done in this incredibly janky way.

@corngood
Copy link
Contributor Author

corngood commented Nov 16, 2025

Sucks that we are using the operator new/delete that just allocate raw bytes. I have no clue why it's done in this incredibly janky way.

As far as I can tell it's to make it easier to init self-references in structs, like:

        return unsafe_new_with_self<nix_eval_state_builder>([&](auto * self) {
            return nix_eval_state_builder{
                .store = nix::ref<nix::Store>(store->ptr),
                .settings = nix::EvalSettings{/* &bool */ self->readOnlyMode},
                .fetchSettings = nix::fetchers::Settings{},
                .readOnlyMode = true,
            };
        });
template<typename T, typename F>
static T * unsafe_new_with_self(F && init)
{
    // Allocate
    void * p = ::operator new(sizeof(T), static_cast<std::align_val_t>(alignof(T)));
    // Initialize with placement new
    return new (p) T(init(static_cast<T *>(p)));
}

It's clever, but it's only used in two places. And obviously it's pretty easy to get wrong, because it was originally mixing different new/delete operators which lead to the fix that caused the leak...

@Ericson2314 Ericson2314 added this pull request to the merge queue Nov 16, 2025
Merged via the queue into NixOS:master with commit aec59a9 Nov 16, 2025
31 checks passed
@edolstra edolstra mentioned this pull request Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c api Nix as a C library with a stable interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants