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

Test with valgrind #8601

Open
roberth opened this issue Jun 29, 2023 · 2 comments
Open

Test with valgrind #8601

roberth opened this issue Jun 29, 2023 · 2 comments
Labels
feature Feature request or proposal performance tests

Comments

@roberth
Copy link
Member

roberth commented Jun 29, 2023

Is your feature request related to a problem? Please describe.

Memory leaks are bad; use after free and other memory problems are worse.

Describe the solution you'd like

Valgrind can check for such issues.

Run tests with valgrind and check the all goes well.

Challenges:

  • The test suite performs many nix invocations
    • Wrap every call? (using $PATH)
    • We probably shouldn't escalate problems through return codes but asynchronously, picked up at the end of the test (though that's more fragile...)
    • Don't run valgrind when building Nix; run it separately in a flake check (checks)
  • We have a conservative garbage collector in most invocations. This is would lead to false positives unless we
    • Treat the collectable heap as roots, which leads to false negatives which is arguably better for an automated test suite.
      • Have a flag to disable this for manual invocations, so we can find leaks of Value objects as well
    • Only run valgrind on non evaluating commands: store level operations mostly
    • Only run valgrind on the daemon

Describe alternatives you've considered

Check all new, strdup, etc invocations by hand. Doesn't seem quite as productive or sustainable.

Additional context

Priorities

Add 👍 to issues you find important.

@roberth roberth added feature Feature request or proposal performance tests labels Jun 29, 2023
@edolstra
Copy link
Member

I have a Valgrind suppressions file for Boehm GC that I've used for running Nix on Valgrind. It's from 2012 though so it probably needs some updating.

@inclyc
Copy link
Member

inclyc commented Jun 29, 2023

AFAIK, testing current nix implementation with vanilla valgrind without any modification will definitely ends up a lot of errors, we are using the bohem GC in nix, and for CLI use, we just rely the OS to free memory.

#8600 was found by a forked parser which do not require nix::EvalState, there is no GC (so we can use valgrind).

Here is the report of nix::EvalState::parseExprFromFile, parsing nixpkgs/pkgs/top-level/all-packages.nix

==1190649== 
==1190649== HEAP SUMMARY:
==1190649==     in use at exit: 7,498,218 bytes in 143,056 blocks
==1190649==   total heap usage: 419,569 allocs, 276,513 frees, 37,469,508 bytes allocated
==1190649== 
==1190649== 120 (72 direct, 48 indirect) bytes in 3 blocks are definitely lost in loss record 141 of 219
==1190649==    at 0x4842EE1: operator new(unsigned long) (in /nix/store/wqj0hxq81ysa3fj07vv16why80iz90rk-valgrind-3.21.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1190649==    by 0x49ADC6E: nix::stripIndentation(nix::PosIdx, nix::SymbolTable&, std::vector<std::pair<nix::PosIdx, std::variant<nix::Expr*, StringToken> >, std::allocator<std::pair<nix::PosIdx, std::variant<nix::Expr*, StringToken> > > >&&) [clone .constprop.0] (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x49B370D: yyuserAction(int, int, yyGLRStackItem*, yyGLRStack*, long, YYSTYPE*, YYLTYPE*, void*, nix::ParseData*) [clone .constprop.0] (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x49BC968: yydoAction(yyGLRStack*, long, int, YYSTYPE*, YYLTYPE*, void*, nix::ParseData*) [clone .isra.0] (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x49C22E5: yyglrReduce(yyGLRStack*, long, int, bool, void*, nix::ParseData*) [clone .isra.0] (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x493760F: yyparse(void*, nix::ParseData*) (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x4937DAE: nix::EvalState::parse(char*, unsigned long, std::variant<nix::Pos::none_tag, nix::Pos::Stdin, nix::Pos::String, nix::SourcePath>, nix::SourcePath const&, std::shared_ptr<nix::StaticEnv>&) (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x49380B6: nix::EvalState::parseExprFromFile(nix::SourcePath const&, std::shared_ptr<nix::StaticEnv>&) (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x450423: main (nix-ast-dump.cpp:54)
==1190649== 
==1190649== 384 bytes in 1 blocks are possibly lost in loss record 163 of 219
==1190649==    at 0x48475F0: calloc (in /nix/store/wqj0hxq81ysa3fj07vv16why80iz90rk-valgrind-3.21.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1190649==    by 0x4011212: allocate_dtv (in /nix/store/flf14c3ibr83jsa070j25hg5gjapydhl-glibc-2.37-8/lib/ld-linux-x86-64.so.2)
==1190649==    by 0x4011C11: _dl_allocate_tls (in /nix/store/flf14c3ibr83jsa070j25hg5gjapydhl-glibc-2.37-8/lib/ld-linux-x86-64.so.2)
==1190649==    by 0x50B3957: pthread_create@@GLIBC_2.34 (in /nix/store/flf14c3ibr83jsa070j25hg5gjapydhl-glibc-2.37-8/lib/libc.so.6)
==1190649==    by 0x4EC4698: std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (in /nix/store/n7pvb7gdf1g6dvj7sl92i882qjl4kyx9-gcc-12.3.0-lib/lib/libstdc++.so.6.0.30)
==1190649==    by 0x4D6A9E3: nix::startSignalHandlerThread() (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixutil.so)
==1190649==    by 0x4DD2698: nix::initNix() (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixmain.so)
==1190649==    by 0x450820: InitNix::InitNix() (nix-ast-dump.cpp:15)
==1190649==    by 0x4503BF: main (nix-ast-dump.cpp:52)
==1190649== 
==1190649== 7,463,249 (40 direct, 7,463,209 indirect) bytes in 1 blocks are definitely lost in loss record 219 of 219
==1190649==    at 0x4842EE1: operator new(unsigned long) (in /nix/store/wqj0hxq81ysa3fj07vv16why80iz90rk-valgrind-3.21.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==1190649==    by 0x49B10B6: yyuserAction(int, int, yyGLRStackItem*, yyGLRStack*, long, YYSTYPE*, YYLTYPE*, void*, nix::ParseData*) [clone .constprop.0] (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x49BC968: yydoAction(yyGLRStack*, long, int, YYSTYPE*, YYLTYPE*, void*, nix::ParseData*) [clone .isra.0] (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x49C22E5: yyglrReduce(yyGLRStack*, long, int, bool, void*, nix::ParseData*) [clone .isra.0] (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x493760F: yyparse(void*, nix::ParseData*) (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x4937DAE: nix::EvalState::parse(char*, unsigned long, std::variant<nix::Pos::none_tag, nix::Pos::Stdin, nix::Pos::String, nix::SourcePath>, nix::SourcePath const&, std::shared_ptr<nix::StaticEnv>&) (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x49380B6: nix::EvalState::parseExprFromFile(nix::SourcePath const&, std::shared_ptr<nix::StaticEnv>&) (in /nix/store/rbm1pzh07byv9s9fr99c2pqmw743jn9s-nix-2.16.1/lib/libnixexpr.so)
==1190649==    by 0x450423: main (nix-ast-dump.cpp:54)
==1190649== 
==1190649== LEAK SUMMARY:
==1190649==    definitely lost: 112 bytes in 4 blocks
==1190649==    indirectly lost: 7,463,257 bytes in 142,662 blocks
==1190649==      possibly lost: 384 bytes in 1 blocks
==1190649==    still reachable: 34,465 bytes in 389 blocks
==1190649==         suppressed: 0 bytes in 0 blocks
==1190649== Reachable blocks (those to which a pointer was found) are not shown.
==1190649== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==1190649== 
==1190649== Use --track-origins=yes to see where uninitialised values come from
==1190649== For lists of detected and suppressed errors, rerun with: -s
==1190649== ERROR SUMMARY: 240 errors from 14 contexts (suppressed: 0 from 0)

roberth added a commit to nixops4/nixops4 that referenced this issue Apr 4, 2024
Remove the paranoid check stuff; not feasible until upstream
implements NixOS/nix#8601
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal performance tests
Projects
None yet
Development

No branches or pull requests

3 participants