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 Issue with recursive folders. #1

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Fix Issue with recursive folders. #1

merged 2 commits into from
Dec 16, 2024

Conversation

johnathan-becker
Copy link

This PR addresses an issue in the NSDirectoryEnumerator where calling skipDescendents prematurely halts the directory enumeration process when the _stack data structure becomes empty. The root cause lies in the logic of skipDescendents, which previously removed the last item from _stack unconditionally when GSIArrayCount(_stack) > 0.

@johnathan-becker
Copy link
Author

@qmfrederik Could you please review this. We have already QA'd it and it fixes the issue.

@qmfrederik
Copy link
Owner

Thanks @johnathan-becker!

Do we know why checking for GSIArrayCount(_stack) > 1 rather than GSIArrayCount(_stack) > 1 fixes the issue?
Looks like GNustep main keeps the check for GSIArrayCount(_stack) > 0 (see https://github.com/gnustep/libs-base/blob/master/Source/NSFileManager.m#L2859). Do you know if we can reproduce this issue with GNUstep main on Linux?

I just want to make sure we're not working around a potentially deeper issue (perhaps another subtle bug in libobjc2?).

Having said all that, since this change seems to be scoped to NSFileManager, I guess the risk for regressions is pretty isolated?

Frederik.

@johnathan-becker
Copy link
Author

@qmfrederik We cannot reproduce the issue with GNUstep main on Linux. You are correct they did not change this on Master, but they also rewrote a lot of this code. The underlying issue might reside in the runtime, I am not sure. If you would like me to investigate further I am happy too we just know this fixes the issue and I do believe is a good change since when you remove the active pointer in skipDescendants you would normally expect undefined behavior.

@qmfrederik
Copy link
Owner

Let's take this as is for now (since it fixes the issue) but certainly something to keep int he back of our minds, I think.

@qmfrederik qmfrederik merged commit 36c2e77 into qmfrederik:ucrt64 Dec 16, 2024
0 of 2 checks passed
@johnathan-becker
Copy link
Author

That seems fair. Thank you for merging this. I am looking to get your changes merged into the gnustep_eggplant_branch today or tomorrow.

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.

2 participants