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

gdb: configure with stdenv.cc.cc.lib safe path #73574

Merged
merged 6 commits into from Nov 27, 2019
Merged

Conversation

tmplt
Copy link
Member

@tmplt tmplt commented Nov 17, 2019

Motivation for this change
$ cat > main.cpp
#include <vector>
int main()
{
    std::vector<int> v{0, 1, 2, 3};
}
$ g++ -g -O0 main.cpp
$ gdb ./a.out
(gdb) print v

Before:

$1 = {<std::_Vector_base<int, std::allocator<int> >> = {_M_impl = {<std::allocator<int>> = {<__gnu_cxx::new_allocator<int>> = {<No data fields>}, <No data fields>}, _M_start = 0x416e70, _M_finish = 0x416e80,
      _M_end_of_storage = 0x416e80}}, <No data fields>}

After:

$1 = std::vector of length 4, capacity 4 = {0, 1, 2, 3}
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @globin

@veprbl
Copy link
Member

veprbl commented Nov 17, 2019

@tmplt Could you please rebase to staging?

@tmplt tmplt changed the base branch from master to staging November 17, 2019 22:38
@tmplt
Copy link
Member Author

tmplt commented Nov 17, 2019

@veprbl done.

Pardon the review requests; forgot to change target branch before I pushed the rebase.

@Ericson2314
Copy link
Member

Ericson2314 commented Nov 18, 2019

This seems good, but in general what are these safe plugins and where do they come from? I ask because long term I want to always use separate derivations for libgcc, libstdc++, etc, just as we do for compiler-rt and libc++, etc (#26004).

@ofborg ofborg bot requested a review from lovek323 November 18, 2019 01:06
@tmplt
Copy link
Member Author

tmplt commented Nov 18, 2019 via email

name = gdb.name;
buildInputs = [ makeWrapper ];
propagatedBuildInputs = [ gdb ];
propagatedUserEnvPkgs = [ gdb ];
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid propagatedUserEnvPkgs. It's not needed either.

Copy link
Member Author

Choose a reason for hiding this comment

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

If removed the man-page for gdb is not available. Is there an alternative?

Copy link
Member

Choose a reason for hiding this comment

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

The wrapper could become part of the package itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I should apply my changes in gdb/default.nix instead?

Copy link
Member

Choose a reason for hiding this comment

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

For sake of simplicity I would actually prefer gdb not being a script.
Instead I would suggest to compile stdenv.cc.cc.lib as default safe path into gcc: https://sourceware.org/gdb/onlinedocs/gdb/Auto_002dloading-safe-path.html using --with-auto-load-safe-path. Since libgcc is a dependency of gdb this would not increase closure size. There might be other packages that could be of interests such as go in safe-path. However I would rather put this into a nixos/homemanager module since not everybody uses those.

Copy link
Member

Choose a reason for hiding this comment

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

Personally I just have the whole /nix/store in my safe path, since I trust the programs that I am debugging in general.

Copy link
Member Author

Choose a reason for hiding this comment

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

--with-auto-load-safe-path

Ah, so there is a configure-time option for this; I'll try it out.

I just have the whole /nix/store

Does this not require you to trust your whole store? I don't, so I have ad-hoc white-listed the libstd++ plugin every update before this patch.

Copy link
Member

@Mic92 Mic92 Nov 19, 2019

Choose a reason for hiding this comment

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

@tmplt right, however I personally don't consider this a problem, since I don't attach GDB to random processes and it depends on the libraries to linked to a process what plugins gdb would load. Also I don't quite share the threat model of GDB. If I execute a malicious program the system would be screwed anyway. An malicious gdb plugin does not add much. It would be only a problem if GDB would attach to an untrusted process, which is not what most people do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Latest commit adds a configuration flag and scaps the wrapper.

pkgs/development/tools/misc/gdb/wrapper.nix Outdated Show resolved Hide resolved
pkgs/development/tools/misc/gdb/wrapper.nix Outdated Show resolved Hide resolved
buildInputs = [ makeWrapper ];
propagatedBuildInputs = [ gdb ];
propagatedUserEnvPkgs = [ gdb ];
phases = "installPhase fixupPhase";
Copy link
Member

Choose a reason for hiding this comment

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

Avoid setting phases. Instead, pass e.g. dontBuild = true;

Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto: using something like buildCommand instead doesn't make the man-page available.

@FRidh FRidh moved this from Needs review to WIP in Staging Nov 19, 2019
@Mic92
Copy link
Member

Mic92 commented Nov 19, 2019

This seems good, but in general what are these safe plugins and where do they come from? I ask because long term I want to always use separate derivations for libgcc, libstdc++, etc, just as we do for compiler-rt and libc++, etc (#26004).

They are provided by the packages itself. i.e.:

$ file $(nix-build --no-out-link '<nixpkgs>' -A stdenv.cc.cc.lib)/lib/libstdc++.so.*-gdb.py

Gdb looks for them next to the library and load them if their path is white-listed.

@ofborg ofborg bot requested review from globin and nbp November 19, 2019 20:55
@tmplt tmplt changed the title gdb: wrap, making libstdc++ plugin safe to load gdb: configure with stdenv.cc.cc.lib safe path Nov 21, 2019
@tmplt tmplt requested review from Mic92 and FRidh November 21, 2019 17:06
@Ericson2314
Copy link
Member

Ericson2314 commented Nov 25, 2019

@Mic92 if this looks good to you, it looks good to me.

Actually wait, I am not sure about with-auto-load-safe-path because it makes our previously target-agnostic gdb target sensitive.

@Mic92
Copy link
Member

Mic92 commented Nov 25, 2019

@Ericson2314 can you elaborate on what you mean with target sensitive? With this PR GDB marks its own libgcc as a safe path. A binary that has a different libgcc needs an additional whitelisting - however we can not solve this here.

Co-Authored-By: Jörg Thalheim <Mic92@users.noreply.github.com>
@Mic92 Mic92 merged commit 3ebebaf into NixOS:staging Nov 27, 2019
Staging automation moved this from WIP to Done Nov 27, 2019
@Ericson2314
Copy link
Member

Is the intent that you are whitlisiting the libgcc that built gdb, or the libgcc that's probably used in the executable being debugged?

@tmplt
Copy link
Member Author

tmplt commented Nov 28, 2019 via email

@Ericson2314
Copy link
Member

Ericson2314 commented Nov 28, 2019

So this makes it definitionally the one that built gdb, while targetPackages.stdenv.cc.cc.libc would definitionally be the latter. The former is not what is wanted, thought at least not target sensative. The latter is what we want, but is sadly target sensitive.

I would rather revert this and have a db-wrapper, like cc-wrapper and bintools-wrapper, for gdb and lldb that injected the right flag for targetPackages.stdenv.cc.cc.lib without rebuilding gdb and lldb.

CC @matthewbauer

@Mic92
Copy link
Member

Mic92 commented Nov 29, 2019

@Ericson2314 is stdenv.cc.cc.lib not the libgcc that is used at runtime? Because it is used everywhere in nixpkgs.

@Ericson2314
Copy link
Member

@Mic92 It is, but each stage has it's own (potentially different) stdenv.cc.cc.lib. Tools are gotten from buildPackages, and so we have buildPackages.targetPackages.stdenv.cc.cc.lib and so the buildPackages.targetPackages cancels out and we get back to stdenv.cc.cc.lib.

@Ericson2314
Copy link
Member

Ah i was totally wrong, gdb is not currently target agnostic, so the fix is very minimal: #74806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants