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

Stop at the first NULL argument when iterating argv #106001

Merged
merged 1 commit into from
Feb 11, 2023
Merged

Stop at the first NULL argument when iterating argv #106001

merged 1 commit into from
Feb 11, 2023

Conversation

sdroege
Copy link
Contributor

@sdroege sdroege commented Dec 21, 2022 •
edited
Loading

Some C commandline parsers (e.g. GLib and Qt) are replacing already handled arguments in argv with NULL and move them to the end. That means that argc might be bigger than the actual number of non-NULL pointers in argv at this point.

To handle this we simply stop iterating at the first NULL argument.

argv is also guaranteed to be NULL-terminated so any non-NULL arguments after the first NULL can safely be ignored.

Fixes #105999

@rustbot
Copy link
Collaborator

rustbot commented Dec 21, 2022

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 21, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 21, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@sdroege sdroege mentioned this pull request Dec 21, 2022
Closed
@sdroege
Stop at the first NULL argument when iterating argv
e97203c 
Some C commandline parsers (e.g. GLib and Qt) are replacing already
handled arguments in `argv` with `NULL` and move them to the end. That
means that `argc` might be bigger than the actual number of non-`NULL`
pointers in `argv` at this point.

To handle this we simply stop iterating at the first `NULL` argument.

`argv` is also guaranteed to be `NULL`-terminated so any non-`NULL`
arguments after the first `NULL` can safely be ignored.

Fixes rust-lang#105999
@sdroege sdroege changed the title Skip over NULL arguments on Linux/glibc when iterating argv Stop at the first NULL argument when iterating argv Dec 23, 2022
@sdroege
Copy link
Contributor Author

sdroege commented Jan 25, 2023

Is there anything I can help to move this forward? It would be great if this wouldn't cause crashes anymore 😅

@sdroege
Copy link
Contributor Author

sdroege commented Feb 6, 2023

@joshtriplett As this basically implements what you suggested in the issues and this is assigned to you for review, is there anything I can help with to get this reviewed faster?

@RalfJung
Copy link
Member

Might be worth re-rolling the reviewer dice.

r? libs

@rustbot rustbot assigned thomcc and unassigned joshtriplett Feb 10, 2023
@ChrisDenton
Copy link
Member

Code looks good to me. I happy to accept this given this is a potential safety issue (or at least this PR is better than crashing). And josh's opinion that it's a perfectly valid way to parse argv:

To the best of my knowledge, it's completely valid to parse argv by stopping at the first NULL; if an argument parser is replacing an argument with NULL, it should not expect that arguments after that point will be seen. I've seen plenty of C programs that just do for (char **arg = argv; *arg; arg++) or equivalent.

Argument parsers mutating argv sounds like a very dicey situation and Rust's std should play it safe.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 10, 2023

📌 Commit e97203c has been approved by ChrisDenton

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2023
@Dylan-DPC Dylan-DPC mentioned this pull request Feb 11, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2023
@bors
Auto merge of rust-lang#107919 - Dylan-DPC:rollup-fkl9swa, r=Dylan-DPC
71f6675 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#105019 (Add parentheses properly for borrowing suggestion)
 - rust-lang#106001 (Stop at the first `NULL` argument when iterating `argv`)
 - rust-lang#107098 (Suggest function call on pattern type mismatch)
 - rust-lang#107490 (rustdoc: remove inconsistently-present sidebar tooltips)
 - rust-lang#107855 (Add a couple random projection tests for new solver)
 - rust-lang#107857 (Add ui test for implementation on projection)
 - rust-lang#107878 (Clarify `new_size` for realloc means bytes)
 - rust-lang#107888 (revert rust-lang#107074, add regression test)
 - rust-lang#107900 (Zero the `REPARSE_MOUNTPOINT_DATA_BUFFER` header)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0e8f0b0 into rust-lang:master Feb 11, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 11, 2023
@sdroege sdroege deleted the glibc-skip-over-null-argv branch February 11, 2023 17:47
@sdroege
Copy link
Contributor Author

sdroege commented Feb 11, 2023

Can this also be backported to beta so it ends up in 1.68?

@RalfJung
Copy link
Member

We usually only backport regression fixes I think. @Mark-Simulacrum do you think this qualifies for a backport?

@Mark-Simulacrum
Copy link
Member

It's up to T-libs, but I would lean towards no. We land bugfixes routinely and don't typically backport them. This is a soundness fix I guess, but only in edge cases, so I don't think it should merit special consideration.

@RalfJung
Copy link
Member

@rust-lang/libs should this be backported to beta?

@ChrisDenton ChrisDenton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 12, 2023
@ChrisDenton
Copy link
Member

I've added the "beta-nominated" label to make sure it gets discussed. This will indeed need t-libs sign off before being accepted (or not).

@m-ou-se m-ou-se removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 22, 2023
unknowndevQwQ added a commit to unknowndevQwQ/killmyargv that referenced this pull request Feb 7, 2024
@unknowndevQwQ
fix: iterating stop at the first NULL argument
b10937f 
Do it like rust-lang/rust#106001
@madsmtm madsmtm mentioned this pull request May 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb left review comments

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@thomcc thomcc

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.

glibc .init_array usage for std::env::args is not safe