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

libredirect: fix argument forwarding in `open*` functions #73266

Merged
merged 2 commits into from Dec 9, 2019

Conversation

@demin-dmitriy
Copy link
Contributor

@demin-dmitriy demin-dmitriy commented Nov 12, 2019

Motivation for this change

libredirect incorrectly forwards arguments of open, open64 and openat functions when O_TMPFILE flag is set.

From man 2 open:

The mode argument specifies the file mode bits be applied when a new file is created. This argument must be supplied when O_CREAT or O_TMPFILE is specified in flags; if neither O_CREAT nor O_TMPFILE is specified, then mode is ignored.

This PR solves this issue: #73077 .

Things done
  • Fixed argument forwarding in open, open64, openat functions
  • Fixed return type of access function
  • 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 @Ma27

Flag `O_TMPFILE` was added in Linux 3.11. It affects whether or not
`mode` argument should be passed.
`access` should return `int` not `int*`. Actually compiler produced
identical assembly with any of those types, so by luck it "just worked".
@demin-dmitriy demin-dmitriy changed the title Fix libredirect open bug libredirect: fix argument forwarding in `open*` functions Nov 12, 2019
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Nov 12, 2019

Could you add a testcase for this?

@Ma27
Ma27 approved these changes Nov 12, 2019
Copy link
Member

@Ma27 Ma27 left a comment

Diff seems fine, the issue is fixed in the KDE VM I created to reproduce this. Thanks!

@demin-dmitriy
Copy link
Contributor Author

@demin-dmitriy demin-dmitriy commented Nov 13, 2019

@jtojnar I'm not sure if I can write a good test that tests that O_TMPFILE is working.
The problem is that O_TMPFILE is filesystem-dependent.
From man 2 open:

O_TMPFILE requires support by the underlying filesystem; only a subset of Linux filesystems provide that support. In the initial implementation, support was provided in the ext2, ext3, ext4, UDF, Minix, and shmem filesystems. Support for other filesystems has subsequently been added as follows: XFS (Linux 3.15); Btrfs (Linux 3.16); F2FS (Linux 3.16); and ubifs (Linux 4.9).

So either there is a guarantee that during test a file within supported filesystem can be created, or we test only if we can and silently fail if unsupported filesystem is used. In the later case I wonder if that counts as breaking reproducibility.

Copy link
Contributor

@jonringer jonringer left a comment

nix-review passes on NixOS (failures are broken on master)
diff LGTM

[29 built (6 failed), 57 copied (270.3 MiB), 65.2 MiB DL]
error: build of '/nix/store/ibwl6l7pl6prp1c613ldkc2zj9b9g0mr-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/73266
9 package failed to build:
citrix_workspace citrix_workspace_19_3_0 citrix_workspace_19_6_0 citrix_workspace_19_8_0 freeoffice gdbgui python37Packages.flask-socketio python37Packages.python-engineio python37Packages.python-socketio

10 package were build:
libredirect python38Packages.flask-socketio python38Packages.python-engineio python38Packages.python-socketio softmaker-office sublime-merge sublime-merge-dev sublime3 sublime3-dev teamspeak_client

didn't do any testing

@demin-dmitriy
Copy link
Contributor Author

@demin-dmitriy demin-dmitriy commented Nov 25, 2019

@Ma27 Let's merge this? (I don't know if there a reason not to).

Copy link
Contributor

@jonringer jonringer left a comment

nix-review passes on NixOS
diff LGTM
commits LGTM

[27 built (4 failed), 19 copied (73.3 MiB), 12.4 MiB DL]
error: build of '/nix/store/njfx7kk2smycjy2xfsqpwk2yh3268dd0-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/73266
4 package failed to build:
citrix_workspace citrix_workspace_19_3_0 citrix_workspace_19_6_0 citrix_workspace_19_8_0

15 package were built:
freeoffice gdbgui libredirect python37Packages.flask-socketio python37Packages.python-engineio python37Packages.python-socketio python38Packages.flask-socketio python38Packages.python-engineio python38Packages.python-socketio softmaker-office sublime-merge sublime-merge-dev sublime3 sublime3-dev teamspeak_client
@jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 25, 2019

@GrahamcOfBorg build libredirect

@jtojnar jtojnar merged commit 6b73d29 into NixOS:master Dec 9, 2019
16 checks passed
16 checks passed
libredirect on x86_64-darwin Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
libredirect on aarch64-linux Success
Details
libredirect on x86_64-linux Success
Details
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Dec 9, 2019

I guess we will have to do without test. Thanks.

dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Dec 10, 2019
…n-bug

libredirect: fix argument forwarding in `open*` functions
(cherry picked from commit 6b73d29)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.