Skip to content

Conversation

@marktucker
Copy link
Contributor

Fix another issue with unresolvable reparse points on Windows. ArchReadLink
can return the passed in path if it refers to an "NT Object Manager" path, and we need to handle this as if it isn't a reparse point, because we can't follow the link.

This generalizes the fix by @seando-adsk in f6c2e82, by handling mounted volumes on Windows and avoiding infinitely recursive calls to TfReadLink by having TfIsLink return false in cases where this bad behavior could arise.

Description of Change(s)

Changed Tf_HasAttribute to remove the reparse point attribute on a directory if TfReadLink returns the original path or an empty string (i.e. we couldn't follow the link). This means TfIsLink returns false in these cases, preventing recursive calls to TfReadLink because we always call TfIsLink before calling TfReadLink.

Fixes Issue(s)

#3414

Checklist

[ X ] I have created this PR based on the dev branch

[ X ] I have followed the coding conventions

[ ] I have added unit tests that exercise this functionality
Unit testing this is basically impossible given the complex steps required to set up the USB drive appropriately.

[ X ] I have verified that all unit tests pass with the proposed changes

[ X ] I have submitted a signed Contributor License Agreement (Reference:

…adLink

can return the passed in path if it refers to an "NT Object Manager" path,
and we need to handle this as if it isn't a reparse point, because we can't
follow the link.

This generalizes the fix by @seando-adsk in f6c2e82, by handling mounted
volumes on Windows and avoiding infinitely recursive calls to TfReadLink
by having TfIsLink return false in cases where this bad behavior
could arise.
@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-10419

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@asluk asluk added the needs review Issue needing input/review by the repo maintainer (Pixar) label Nov 18, 2024
Copy link
Contributor

@anwang2009 anwang2009 left a comment

Choose a reason for hiding this comment

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

Thanks @marktucker, this PR looks great - just one comment

// Read symlinks until we find the real file.
return Tf_HasAttribute(TfReadLink(path.c_str()), resolveSymlinks,
attribute, expected);
return Tf_HasAttribute(linkPath, resolveSymlinks, attribute, expected);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit tricky to tell that linkPath is non-empty from reading the code; it'd be great if that could be made more obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is an additional comment around line 117 sufficient? Something like:

// At this point we know (attribs & FILE_ATTRIBUTE_REPARSE_POINT) != 0
// or we would have returned in the if block above. This means linkPath will
// be holding the result of calling TfReadLink(path.c_str()). The code is separated
// in this way to avoid calling TfReadLink twice.

@anwang2009
Copy link
Contributor

anwang2009 commented Feb 26, 2025 via email

Tf_HasAttribute to address confusion expressed in the PR review.
@pixar-oss pixar-oss closed this in ae357f7 Mar 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs review Issue needing input/review by the repo maintainer (Pixar)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants