Skip to content

gh-134873: Fix a DOS issue in posixpath #134927

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
Closed
wants to merge 1 commit into from
Closed

gh-134873: Fix a DOS issue in posixpath #134927

wants to merge 1 commit into from
+13 −8

Conversation

Wulian233
Copy link
Contributor

@Wulian233 Wulian233 commented May 30, 2025 •
edited by bedevere-app bot
Loading 

import time
import os
import posixpath
import Lib.posixpath as posixpath_fixed

os.environ['A'] = 'x'
payload = "$A" * 100000

print("Payload size:", len(payload))

start = time.time()
posixpath.expandvars(payload)
print("Original time:", time.time() - start, "seconds")

start = time.time()
posixpath_fixed.expandvars(payload)
print("Fixed time:", time.time() - start, "seconds")

Above is a code that reproduces the problem, and the speed difference becomes more and more noticeable with the number of times. The above 200,000 times is 15x faster

print:

Payload size: 200000
Original time: 1.7329566478729248 seconds
Fixed time: 0.11450839042663574 seconds

@bedevere-app bedevere-app bot mentioned this pull request May 30, 2025
Open
@ZeroIntensity ZeroIntensity added type-security A security issue needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels May 30, 2025
@serhiy-storchaka
Copy link
Member

There is other quadratic complexity here -- in the regular expression. Example: expandvars('${'*1000000).

Also, using re.sub() is more efficient here. See #134952.

@Wulian233
Copy link
Contributor Author

Also, using re.sub() is more efficient here. See #134952

#134952 include this fix. What should I do? Should I modify this PR according to your changes or close this PR👀

@serhiy-storchaka
Copy link
Member

I suggest to close this PR in favor of #134952, unless the latter has serious flaws. Thank you anyway.

@Wulian233
Copy link
Contributor Author

No problem

@Wulian233 Wulian233 closed this May 31, 2025
@Wulian233 Wulian233 deleted the posixpath-dos branch May 31, 2025 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
awaiting review needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes type-security A security issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Wulian233 @serhiy-storchaka @ZeroIntensity