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.

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

fix: resolve infinite loop in _find_config on Windows systems #4970

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Julfried
Copy link

@Julfried Julfried commented Dec 17, 2024

Issue #4967

Description of changes:
This PR fixes an infinite loop in _find_config that occurs on Windows systems. The issue was caused by using Path.match("/") to detect root directories, which never matches on Windows due to different path separators.

Changes made:

  1. Replace platform-specific Path.match("/") with Path.anchor comparison
  2. Implement proper root directory detection for both Windows and Unix systems
  3. Add comprehensive cross-platform tests

Testing done:

  • Verified fix on both Windows and Linux systems
  • Added test_find_config_cross_platform to validate:
    • Basic path traversal
    • Nested directory structures
    • Root directory handling
    • Non-existent config scenarios
  • Added test_find_config_path_separators to verify handling of:
    • Forward slashes
    • Backslashes
    • Mixed separators
  • All existing tests pass without modification

Merge Checklist

General

  • I have read the CONTRIBUTING doc
  • I certify that the changes I am introducing will be backward compatible
  • I used the commit message format described in CONTRIBUTING
  • I have passed the region in to all S3 and STS clients (N/A for this change)
  • I have updated necessary documentation (N/A - internal implementation change)

Tests

  • I have added tests that prove my fix is effective
  • I have added unit tests to ensure backward compatibility
  • I have checked that my tests are not configured for a specific region or account
  • I have used unique_name_from_base for resource names (N/A - unit tests only)
  • No new dependencies added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* Replace Path.match("/") with Path.anchor comparison
* Fix infinite loop in _studio.py path traversal
@Julfried Julfried requested a review from a team as a code owner December 17, 2024 16:08
@Julfried Julfried requested a review from nargokul December 17, 2024 16:08
@Julfried
Copy link
Author

Quick update: I was unsure about two checklist items and checked them with N/A, because I think they are not applicable for my changes:

  1. "Passed region to S3/STS clients"
  2. "Documentation updated"

Please let me know if there is anything I need to do, to get this merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant