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

dmd.root.filename: Fix FileName.canonicalPath() regression on Windows #11293

Merged
merged 1 commit into from
Jun 19, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 28 additions & 17 deletions src/dmd/root/filename.d
Original file line number Diff line number Diff line change
Expand Up @@ -930,7 +930,7 @@ nothrow:
}

/******************************************
* Return canonical version of name in a malloc'd buffer.
* Return canonical version of name.
* This code is high risk.
*/
extern (C++) static const(char)* canonicalName(const(char)* name)
Expand Down Expand Up @@ -975,14 +975,16 @@ nothrow:
char[PATH_MAX] buf = void;
auto path = name.toCStringThen!((n) => realpath(n.ptr, buf.ptr));
if (path !is null)
return mem.xstrdup(path).toDString;
return xarraydup(path.toDString);
}
else static if (__traits(compiles, canonicalize_file_name))
{
// Have canonicalize_file_name, which malloc's memory.
// We need a dmd.root.rmem allocation though.
auto path = name.toCStringThen!((n) => canonicalize_file_name(n.ptr));
scope(exit) .free(path.ptr);
if (path !is null)
return path.toDString;
return xarraydup(path.toDString);
Comment on lines +985 to +987
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment, e.g. // Make sure we consistently return Mem-allocated memory. Otherwise someone is bound to see this and think it can be trivially improved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To prevent such an 'improvement', I've added the FileName.free() call in the test. But that needs an enabled GC (-lowmem) to hopefully produce an error.

}
else static if (__traits(compiles, _PC_PATH_MAX))
{
Expand All @@ -994,14 +996,14 @@ nothrow:
scope(exit) mem.xfree(buf);
auto path = name.toCStringThen!((n) => realpath(n.ptr, buf));
if (path !is null)
return mem.xstrdup(path).toDString;
return xarraydup(path.toDString);
}
}
// Give up trying to support this platform, just duplicate the filename
// unless there is nothing to copy from.
if (!name.length)
return null;
return mem.xstrdup(name.ptr)[0 .. name.length];
return xarraydup(name);
}
else version (Windows)
{
Expand All @@ -1011,18 +1013,18 @@ nothrow:
/* Apparently, there is no good way to do this on Windows.
* GetFullPathName isn't it, but use it anyway.
*/
// First find out how long the buffer has to be.
const fullPathLength = GetFullPathNameW(&wname[0], 0, null, null);
if (!fullPathLength) return null;
auto fullPath = (cast(wchar*) mem.xmalloc_noscan((fullPathLength + 1) * wchar.sizeof))[0 .. fullPathLength + 1];
scope(exit) mem.xfree(fullPath.ptr);

// Actually get the full path name
const length = GetFullPathNameW(
&wname[0], cast(DWORD) fullPath.length, &fullPath[0], null /*filePart*/);
assert(length == fullPathLength);

return toNarrowStringz(fullPath[0 .. length]);
// First find out how long the buffer has to be, incl. terminating null.
const capacity = GetFullPathNameW(&wname[0], 0, null, null);
if (!capacity) return null;
auto buffer = cast(wchar*) mem.xmalloc_noscan(capacity * wchar.sizeof);
scope(exit) mem.xfree(buffer);

// Actually get the full path name. If the buffer is large enough,
// the returned length does NOT include the terminating null...
const length = GetFullPathNameW(&wname[0], capacity, buffer, null /*filePart*/);
assert(length == capacity - 1);

return toNarrowStringz(buffer[0 .. length]);
});
}
else
Expand All @@ -1031,6 +1033,15 @@ nothrow:
}
}

unittest
{
string filename = "foo.bar";
const path = canonicalName(filename);
scope(exit) free(path.ptr);
assert(path.length >= filename.length);
assert(path[$ - filename.length .. $] == filename);
}

/********************************
* Free memory allocated by FileName routines
*/
Expand Down