This is an archive of the discontinued LLVM Phabricator instance.

Windows rename_internal function incorrectly passing character count instead of byte count to SetFileInformationByHandle.
ClosedPublic

Authored by benhillis on Dec 12 2018, 3:13 PM.

Details

Summary

The rename_internal function used for Windows has a minor bug where the filename length is passed as a character count instead of a byte count. Windows internally ignores this field, but other tools that hook NT api's may use the documented behavior:

MSDN documentation specifying the size should be in bytes:
https://docs.microsoft.com/en-us/windows/desktop/api/winbase/ns-winbase-_file_rename_info

Diff Detail

Event Timeline

benhillis created this revision.Dec 12 2018, 3:13 PM

Added a few reviewers based on git blame, please feel free to forward to anybody who would be more appropriate.

smeenai added a subscriber: smeenai.

This LGTM, but before I approve, I'd like to look around the surrounding code to confirm things are how I expect, but this patch is uploaded without context :( Could you reupload with context? See https://llvm.org/docs/Phabricator.html (the part about using -U999999 in your git commands to generate full context).

benhillis added a comment.EditedDec 12 2018, 3:50 PM

Ah, my mistake. I will update the patch.

smeenai accepted this revision.Dec 12 2018, 3:56 PM

That's the intent ... it uploads the entire file to Phabricator, so that it can be viewed through the web interface. The arcanist tool does that for you automatically, as an alternative to have to copy and paste a giant block of text around.

In this case, I just looked at the surrounding context myself, and this LGTM. Do you need someone to commit this? Context will be helpful for future patches though.

This revision is now accepted and ready to land.Dec 12 2018, 3:56 PM
benhillis updated this revision to Diff 177964.EditedDec 12 2018, 3:58 PM

@smeenai - Thanks for the explanation, I misunderstood what the -U999999 was doing. Yes if this patch is accepted, I'll need somebody to commit it for me to the LLVM mainline.

This revision was automatically updated to reflect the committed changes.

Committed in r348995. (I also took the liberty of adding a [Support] tag to the title and wrapping the commit message, in line with LLVM conventions; see https://llvm.org/docs/DeveloperPolicy.html#commit-messages). Thanks for the patch!