Skip to content

Panic if PathBuf::set_extension would add a path separator#125070

Merged
bors merged 1 commit into
rust-lang:masterfrom
tbu-:pr_set_extension_panic
May 26, 2024
Merged

Panic if PathBuf::set_extension would add a path separator#125070
bors merged 1 commit into
rust-lang:masterfrom
tbu-:pr_set_extension_panic

Conversation

@tbu-

@tbu- tbu- commented May 13, 2024

Copy link
Copy Markdown
Contributor

This is likely never intended and potentially a security vulnerability if it happens.

I'd guess that it's mostly literal strings that are passed to this function in practice, so I'm guessing this doesn't break anyone.

CC #125060

@rustbot

rustbot commented May 13, 2024

Copy link
Copy Markdown
Collaborator

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 May 13, 2024
@tbu- tbu- mentioned this pull request May 13, 2024
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tbu-
Panic if PathBuf::set_extension would add a path separator
700b3ea 
This is likely never intended and potentially a security vulnerability
if it happens.

I'd guess that it's mostly literal strings that are passed to this
function in practice, so I'm guessing this doesn't break anyone.

CC rust-lang#125060
@tbu- tbu- force-pushed the pr_set_extension_panic branch from 3382993 to 700b3ea Compare May 13, 2024 13:08
ptrca
ptrca reviewed May 13, 2024
View reviewed changes
Comment thread library/std/src/path/tests.rs
}

#[test]
#[should_panic = "path separator"]

@ptrca ptrca May 13, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would test a try_set method be better than testing a panic one?

@jhpratt

jhpratt commented May 14, 2024

Copy link
Copy Markdown
Member

imo this needs a decision from T-libs (or T-libs-api? not sure). I personally find this reasonable, but it's not a trivial thing to change.

cc @rust-lang/libs

@workingjubilee workingjubilee added the I-libs-nominated Nominated for discussion during a libs team meeting. label May 14, 2024
@workingjubilee

workingjubilee commented May 14, 2024

Copy link
Copy Markdown
Member

this and #125060 should probably be discussed together

@ChrisDenton

ChrisDenton commented May 14, 2024

Copy link
Copy Markdown
Member

I think this (and the other one) are an API issue. This isn't just about the implementation. This is about the public API of Path.

@workingjubilee workingjubilee added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 14, 2024
@m-ou-se m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 15, 2024
@joshtriplett

joshtriplett commented May 15, 2024

Copy link
Copy Markdown
Member

We discussed this in today's @rust-lang/libs meeting. We were generally in favor of panicking in set_extension here.

(We were hesitant to advocate the same for set_file_name because there might be existing code out there that depends on being able to pass things with path separators there. Though if we were confident that were not the case we'd love to do that too.)

@rfcbot merge

@rfcbot

rfcbot commented May 15, 2024 •
edited by BurntSushi
Loading

Copy link
Copy Markdown

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 15, 2024
@Amanieu Amanieu removed I-libs-nominated Nominated for discussion during a libs team meeting. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels May 15, 2024
@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 15, 2024
@rfcbot

rfcbot commented May 15, 2024

Copy link
Copy Markdown

🔔 This is now entering its final comment period, as per the review above. 🔔

@tbu-

This comment was marked as off-topic.

Sign in to view
jhpratt
jhpratt approved these changes May 15, 2024
View reviewed changes

@jhpratt jhpratt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

r=me upon FCP completion

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 25, 2024
@dtolnay

dtolnay commented May 25, 2024

Copy link
Copy Markdown
Member

@bors r=jhpratt

@bors

bors commented May 25, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 700b3ea has been approved by jhpratt

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 May 25, 2024
fmease added a commit to fmease/rust that referenced this pull request May 26, 2024
@fmease
Rollup merge of rust-lang#125070 - tbu-:pr_set_extension_panic, r=jhp…
f6e46b4 
…ratt

Panic if `PathBuf::set_extension` would add a path separator

This is likely never intended and potentially a security vulnerability if it happens.

I'd guess that it's mostly literal strings that are passed to this function in practice, so I'm guessing this doesn't break anyone.

CC rust-lang#125060
@fmease fmease mentioned this pull request May 26, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2024
@bors
Auto merge of rust-lang#125563 - fmease:rollup-ioszhmm, r=fmease
6252324 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#125070 (Panic if `PathBuf::set_extension` would add a path separator)
 - rust-lang#125307 (tidy: stop special-casing tests/ui entry limit)
 - rust-lang#125375 (Create a triagebot ping group for Rust for Linux)
 - rust-lang#125413 (drop region constraints for ambiguous goals)
 - rust-lang#125433 (A small diagnostic improvement for dropping_copy_types)
 - rust-lang#125508 (Stop SRoA'ing `DynMetadata` in MIR)
 - rust-lang#125530 (cleanup dependence of `ExtCtxt` in transcribe when macro expansion)
 - rust-lang#125544 (Also mention my-self for other check-cfg docs changes)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors

bors commented May 26, 2024

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 700b3ea with merge f016754...

bors added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2024
@bors
Auto merge of rust-lang#125070 - tbu-:pr_set_extension_panic, r=jhpratt
f016754 
Panic if `PathBuf::set_extension` would add a path separator

This is likely never intended and potentially a security vulnerability if it happens.

I'd guess that it's mostly literal strings that are passed to this function in practice, so I'm guessing this doesn't break anyone.

CC rust-lang#125060
@fmease

fmease commented May 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

@bors retry (yield to rollup which includes this PR and which just failed due to network issues)

bors added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2024
@bors
Auto merge of rust-lang#125563 - fmease:rollup-ioszhmm, r=fmease
7052f31 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#125070 (Panic if `PathBuf::set_extension` would add a path separator)
 - rust-lang#125307 (tidy: stop special-casing tests/ui entry limit)
 - rust-lang#125375 (Create a triagebot ping group for Rust for Linux)
 - rust-lang#125413 (drop region constraints for ambiguous goals)
 - rust-lang#125433 (A small diagnostic improvement for dropping_copy_types)
 - rust-lang#125508 (Stop SRoA'ing `DynMetadata` in MIR)
 - rust-lang#125530 (cleanup dependence of `ExtCtxt` in transcribe when macro expansion)
 - rust-lang#125544 (Also mention my-self for other check-cfg docs changes)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors

bors commented May 26, 2024

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 700b3ea with merge bd184cc...

@bors

bors commented May 26, 2024

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-actions
Approved by: jhpratt
Pushing bd184cc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 26, 2024
@bors bors merged commit bd184cc into rust-lang:master May 26, 2024
@rustbot rustbot added this to the 1.80.0 milestone May 26, 2024
@rust-timer

rust-timer commented May 26, 2024

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (bd184cc): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.2%, secondary 0.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.2% [0.0%, 0.5%] 11
Regressions ❌
(secondary)		 0.5% [0.3%, 0.6%] 38
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.2% [0.0%, 0.5%] 11

Bootstrap: 672.481s -> 671.39s (-0.16%)
Artifact size: 315.71 MiB -> 315.55 MiB (-0.05%)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 7, 2024
@workingjubilee workingjubilee added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 7, 2024
@ryoqun ryoqun mentioned this pull request Aug 15, 2024
Merged
This was referenced Jan 16, 2025
Closed
Closed
@ChrisDenton ChrisDenton linked an issue Jan 22, 2025 that may be closed by this pull request
PathBuf::set_extension accepts an extension containing slashes #32497
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dtolnay dtolnay dtolnay approved these changes

@jhpratt jhpratt jhpratt approved these changes

+1 more reviewer

@ptrca ptrca ptrca left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jhpratt jhpratt

Labels

disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Milestone

1.80.0