Skip to content

Commit

Permalink
dllinject: Track actual attribute writes instead of the requesting flag
Browse files Browse the repository at this point in the history
Opening a file with the `FILE_WRITE_ATTRIBUTES` access right merely *requests*
write access to attributes, and does not constitute a write in itself.
Previously, this caused issues with tools that need to specify this flag for
whatever reason, but didn't actually write or change the attributes of the
files they opened with this flag. If those tools lie outside the source tree,
Tup will in fact delete all of these files, forcing you to either reinstall the
tool or recover the missing files from a backup. (Thankfully, write-protecting
the compiler's directory prevents this from succeeding!)

This change should still address the node.js use case that initially prompted
the check for `FILE_WRITE_ATTRIBUTES` in c7160c8. I couldn't find a definitive
list of everything that counts as "attributes", but it does seem to be limited
to the timestamps and attribute bits covered by the `FILE_BASIC_INFORMATION`
structure.
  • Loading branch information
nmlgc committed May 21, 2024
1 parent 9018985 commit 19e10ed
Showing 1 changed file with 38 additions and 2 deletions.
40 changes: 38 additions & 2 deletions src/dllinject/dllinject.c
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ static NtSetInformationFile_t NtSetInformationFile_orig;
static access_t _access_orig;
static rename_t rename_orig;

#define TUP_CREATE_WRITE_FLAGS (GENERIC_WRITE | FILE_APPEND_DATA | FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES)
#define TUP_CREATE_WRITE_FLAGS (GENERIC_WRITE | FILE_APPEND_DATA | FILE_WRITE_DATA)
/* Including ddk/wdm.h causes other issues, and this is all we need... */
#define FILE_OPEN_FOR_BACKUP_INTENT 0x00004000

Expand Down Expand Up @@ -787,10 +787,43 @@ NTSTATUS WINAPI NtSetInformationFile_hook(
DWORD len = 0;
int failed = 0;

/* * A value of 0 in any field preserves the file's current value for this
* specific attribute.
* * -1 or -2 in the timestamp fields disable or enable the default
* timestamp updates for file I/O:
*
* https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/ns-wdm-_file_basic_information
*
* * SetFileInformationByHandle() returns ERROR_INVALID_PARAMETER for any
* other negative number.
*
* * Only process regular files, as calling handle_file_w() on a directory
* would turn it into a file in tup's database.
*/
FILE_BASIC_INFORMATION* basic = FileInformation;
int relevant_basic_change = (FileInformationClass == FileBasicInformation) && (
(int64_t)(basic->CreationTime.QuadPart) >= 1 ||
(int64_t)(basic->LastAccessTime.QuadPart) >= 1 ||
(int64_t)(basic->LastWriteTime.QuadPart) >= 1 ||
(int64_t)(basic->ChangeTime.QuadPart) >= 1 ||
basic->FileAttributes != 0
);
if(relevant_basic_change) {
BY_HANDLE_FILE_INFORMATION handle_info;
relevant_basic_change = GetFileInformationByHandle(FileHandle, &handle_info);
if(!relevant_basic_change) {
DEBUG_HOOK("NtSetInformationFile Error - failed to GetFileInformationByHandle\n");
failed = 1;
} else {
relevant_basic_change = !(handle_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY);
}
}

/* We need to get the old path before calling NtSetInformationFile in
* case of a rename, in which case that information is lost.
*/
if(FileInformationClass == FileRenameInformation ||
if(relevant_basic_change ||
FileInformationClass == FileRenameInformation ||
FileInformationClass == FileRenameInformationEx ||
FileInformationClass == FileDispositionInformation) {
len = GetFinalPathNameByHandleW(FileHandle, widepath, WIDE_PATH_MAX, FILE_NAME_NORMALIZED);
Expand Down Expand Up @@ -831,6 +864,9 @@ NTSTATUS WINAPI NtSetInformationFile_hook(
DEBUG_HOOK("NtSetInformationFile[%i]: dont delete on close '%S'\n", FileInformationClass, widepath);
handle_file_w(widepath, len, NULL, ACCESS_WRITE);
}
} else if(relevant_basic_change) {
DEBUG_HOOK("NtSetInformationFile[%i]: set basic information '%S'\n", FileInformationClass, widepath);
handle_file_w(widepath, len, NULL, ACCESS_WRITE);
}

out_exit:
Expand Down

0 comments on commit 19e10ed

Please sign in to comment.