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.

Sign up for GitHub

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

Jump to bottom

libredirect: fix argument forwarding in open* functions #73266

Merged
merged 2 commits into from
Dec 9, 2019
Merged

libredirect: fix argument forwarding in open* functions #73266

merged 2 commits into from
Dec 9, 2019
+10 −5

Conversation

demin-dmitriy
Copy link
Contributor

@demin-dmitriy demin-dmitriy commented Nov 12, 2019 •
edited
Loading

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

Sorry, something went wrong.

@demin-dmitriy
libredirect: fix argument forwarding in open* functions

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
6432f92 
Flag `O_TMPFILE` was added in Linux 3.11. It affects whether or not
`mode` argument should be passed.
@demin-dmitriy
libredirect: fix access return type

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
b6e37c3 
`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
Member

jtojnar commented Nov 12, 2019

Could you add a testcase for this?

Ma27
Ma27 approved these changes Nov 12, 2019
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

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 commented Nov 13, 2019 •
edited
Loading

@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.

jonringer
jonringer approved these changes Nov 13, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

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

jonringer
jonringer approved these changes Nov 25, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

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

@GrahamcOfBorg build libredirect

@jtojnar jtojnar merged commit 6b73d29 into NixOS:master Dec 9, 2019
@jtojnar
Copy link
Member

jtojnar commented Dec 9, 2019

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

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Dec 10, 2019
@jtojnar @dtzWill
Merge pull request NixOS#73266 from demin-dmitriy/fix-libredirect-ope…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
58bea52 
…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
Reviewers

@Ma27 Ma27

@jonringer jonringer

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@demin-dmitriy @jtojnar @jonringer @Ma27