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

Replace realpath implementation with libuv #33116

Merged
merged 2 commits into from
Sep 4, 2019
Merged

Conversation

musm
Copy link
Contributor

@musm musm commented Aug 29, 2019

fixes #32597 fixes #32298 fixes #33127

windowserror(:realpath, iszero(len))
resize!(buf, len) # strips NUL terminator on last call
end
if 4 < len < 264 && 0x005c == buf[1] == buf[2] == buf[4] && 0x003f == buf[3]
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I seem to recall that libuv is missing the length check, but otherwise lgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean a max path type length check? I don't think I see that this is an issue.

Copy link
Member

Choose a reason for hiding this comment

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

libuv doesn't check that len > 4, but since it uses wcsncmp (which stops at NUL chars) it should be fine there. It doesn't check len < 264 either, but I guess excluding actual paths with length > 263 that start with \\?\ is less of a practical concern?

Copy link
Contributor Author

@musm musm Sep 1, 2019

Choose a reason for hiding this comment

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

Paths that start with \\?\ are extended paths and can extend up to 32 767 characters. So the len < 264 check should actually not occur.

@musm
Copy link
Contributor Author

musm commented Aug 29, 2019

realpath on master throws SystemError whereas this changes it to Base.IOError which seems more logical.

@musm
Copy link
Contributor Author

musm commented Aug 30, 2019

filewatching failure on win32 and another fluke on freebsd

@musm
Copy link
Contributor Author

musm commented Sep 4, 2019

merge?

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