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

[WIP] Add size-limited command interface, take 2 #82930

Closed
wants to merge 2 commits into from
Closed

[WIP] Add size-limited command interface, take 2 #82930

wants to merge 2 commits into from

Conversation

Artoria2e5
Copy link
Contributor

@Artoria2e5 Artoria2e5 commented Mar 9, 2021 •
edited
Loading

This is a reboot of #74549, which aims to fix #40384. I've changed the thing to use one shared Arg trait, and there is a working-looking xargs impl. The things to do are:

  • Write tests.
    • Write a test that inserts some very short stuff via the new maybe interface and check that they succeed.
    • I want a way to mock the size_max to a smaller value so stuff like xargs are more testable. Maybe later.
    • The Windows raw facility needs testing too.
  • Fix TODO (current CommandSized impl needs to reach down to as_inner_mut()). Should be a quick thing.
  • Fix TODO regarding the weird string-storage behavior on Windows. Should probably just forget about the args vector, but the new force_quote thing will make it the new append-while-adding a sneaky behavior change.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Artoria2e5
Copy link
Contributor Author

Oh right, the notorious doc problem. Heh.

@tavianator
Copy link
Contributor

On Linux at least, you should round up to a multiple of the page size, since the arguments are allocated one page at a time: https://stackoverflow.com/a/46946024/502399

POSIX documents that xargs should subtract an additional 2048 bytes for extra headroom: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/xargs.html

@crlf0710 crlf0710 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2021
@crlf0710 crlf0710 marked this pull request as draft March 26, 2021 15:26
@dtolnay dtolnay removed their assignment Mar 31, 2021
dtolnay
dtolnay reviewed Mar 31, 2021
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Removing assignment. Please r? request review when you need a look.

@Artoria2e5
Copy link
Contributor Author

Artoria2e5 commented Apr 10, 2021 •
edited
Loading

@tavianator Judging from what FreeBSD xargs is doing, it looks like subtracting a whole page size is a surer way to go. Updated the code, will push after check.

@tavianator
Copy link
Contributor

I ran into a situation where ARG_MAX was only two pages, on ppc64le with 64k pages: tavianator/bfs#34. So don't overestimate by too much, but one page is probably okay.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Artoria2e5
Copy link
Contributor Author

I can really use some help with the doc beast...

@bors
Copy link
Contributor

bors commented Apr 12, 2021

☔ The latest upstream changes (presumably #84103) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Artoria2e5
Copy link
Contributor Author

Artoria2e5 commented May 11, 2021 •
edited
Loading

Hmm, some macro_use problems emerged after the latest pull. Time to go lie down and do nothing about it.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented May 18, 2021

☔ The latest upstream changes (presumably #82973) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Artoria2e5 Artoria2e5 mentioned this pull request Jul 10, 2021
Closed
@Artoria2e5
Allow set_errno on Linux
e84a8ca
@Artoria2e5
[WIP] size limit for command line
f9ba5b7 
windows/process: fix some obvious issues, create some more

It looks like I really can't get away with the whole storing error
thing as they don't have Copy. Maybe I will end up using the ugly
problem enum thing like I did with Unix too. Grrr...

unix: incremental size, no strlen

windows: sort the error type out

unix, windows: add temporary dead_code

unix: cache envp

Windows: draft traits for raw args and custom escapers

Windows: raw args trait

Windows: minor rename while I figure out where to put sized

unix: minor error stuff

windows: very rough xargs

minor: run formatter

it wants tight lines!

Windows commandext: apply review suggestions

Attempt at code reuse

Windows builds. Unix not so much.

It builds! Uh.

fixup! re-enable set_errno on linux

cleanup after slighty botched rebase

remove a redundant macro arg

appease tidy

process_common: subtract page size for headroom
@Dylan-DPC
Copy link
Member

Closing this due to inactivity

@Dylan-DPC Dylan-DPC closed this Feb 19, 2022
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay left review comments

Assignees
No one assigned
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

std::process::Command no way to handle command-line length limits
8 participants
@Artoria2e5 @rust-highfive @rust-log-analyzer @tavianator @bors @Dylan-DPC @dtolnay @crlf0710