From fd2a0090957116f5118b38f3c7dc7ab362e91d9d Mon Sep 17 00:00:00 2001 From: denis-sh Date: Wed, 4 May 2011 23:51:16 -0700 Subject: [PATCH 1/4] Issue 5927 fix - Broken getcwd when using GetCurrentDirectoryA --- std/file.d | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/std/file.d b/std/file.d index 4d6fad41f98..f21e142e446 100644 --- a/std/file.d +++ b/std/file.d @@ -1701,11 +1701,12 @@ version(Windows) string getcwd() } else { - auto dir = + auto dirA = new char[enforce(GetCurrentDirectoryA(0, null), "getcwd")]; - dir = dir[0 .. GetCurrentDirectoryA(dir.length, dir.ptr)]; - cenforce(dir.length, "getcwd"); - return assumeUnique(dir); + GetCurrentDirectoryA(dirA.length, dirA.ptr); + string dir = fromMBSz(cast(immutable)dirA.ptr); + enforce(dir.length, "getcwd"); + return dir; } } From 87b3eae25d48e7d27640ea3e41d37d3d2007ef6a Mon Sep 17 00:00:00 2001 From: denis-sh Date: Fri, 27 May 2011 05:45:55 -0700 Subject: [PATCH 2/4] A race hazard in my patch and in phobos code (if compiled with -noboundscheck) is removed. --- std/file.d | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/std/file.d b/std/file.d index f21e142e446..95be71250ed 100644 --- a/std/file.d +++ b/std/file.d @@ -1691,22 +1691,21 @@ version(Windows) string getcwd() // A bit odd API: calling GetCurrentDirectory(0, null) returns // length including the \0, whereas calling with non-zero // params returns length excluding the \0. - if (useWfuncs) + if (std.__fileinit.useWfuncs) { auto dir = new wchar[enforce(GetCurrentDirectoryW(0, null), "getcwd")]; - dir = dir[0 .. GetCurrentDirectoryW(dir.length, dir.ptr)]; - cenforce(dir.length, "getcwd"); - return to!string(dir); + auto n = GetCurrentDirectoryW(dir.length, dir.ptr); + enforce(n && n < dir.length, "getcwd"); + return std.conv.to!string(dir[0 .. n]); } else { - auto dirA = + auto dir = new char[enforce(GetCurrentDirectoryA(0, null), "getcwd")]; - GetCurrentDirectoryA(dirA.length, dirA.ptr); - string dir = fromMBSz(cast(immutable)dirA.ptr); - enforce(dir.length, "getcwd"); - return dir; + auto n = GetCurrentDirectoryA(dir.length, dir.ptr); + enforce(n && n < dir.length, "getcwd"); + return fromMBSz(cast(immutable)dir.ptr); } } From a65927d46a2b496a9d3b86074cfb36c8bdf110d5 Mon Sep 17 00:00:00 2001 From: denis-sh Date: Fri, 27 May 2011 05:48:16 -0700 Subject: [PATCH 3/4] Removed my unnecessary temp "std.__fileinit." --- std/file.d | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/std/file.d b/std/file.d index 95be71250ed..af5b06d3040 100644 --- a/std/file.d +++ b/std/file.d @@ -1691,7 +1691,7 @@ version(Windows) string getcwd() // A bit odd API: calling GetCurrentDirectory(0, null) returns // length including the \0, whereas calling with non-zero // params returns length excluding the \0. - if (std.__fileinit.useWfuncs) + if (useWfuncs) { auto dir = new wchar[enforce(GetCurrentDirectoryW(0, null), "getcwd")]; From 8d6e91d654507ddbda57f58b11e4379ea8750413 Mon Sep 17 00:00:00 2001 From: Denis Date: Sun, 5 Jun 2011 01:47:57 -0700 Subject: [PATCH 4/4] Fixed comment about GetCurrentDirectory's return value No unnecessary heap allocations No silent incorrect behaviour under race hazard --- std/file.d | 54 +++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/std/file.d b/std/file.d index af5b06d3040..0fd481028d9 100644 --- a/std/file.d +++ b/std/file.d @@ -1688,24 +1688,52 @@ version(Posix) void rmdir(in char[] pathname) version(Windows) string getcwd() { - // A bit odd API: calling GetCurrentDirectory(0, null) returns - // length including the \0, whereas calling with non-zero - // params returns length excluding the \0. + /* GetCurrentDirectory's return value: + 1. function succeeds: the number of characters that are written to + the buffer, not including the terminating null character. + 2. function fails: zero + 3. the buffer (lpBuffer) is not large enough: the required size of + the buffer, in characters, including the null-terminating character. + */ + ushort[4096] staticBuff = void; //enough for most common case if (useWfuncs) { - auto dir = - new wchar[enforce(GetCurrentDirectoryW(0, null), "getcwd")]; - auto n = GetCurrentDirectoryW(dir.length, dir.ptr); - enforce(n && n < dir.length, "getcwd"); - return std.conv.to!string(dir[0 .. n]); + auto buffW = cast(wchar[]) staticBuff; + immutable n = cenforce(GetCurrentDirectoryW(buffW.length, buffW.ptr), "getcwd"); + // we can do it because toUTFX always produces a fresh string + if(n < buffW.length) + { + return toUTF8(buffW[0 .. n]); + } + else //staticBuff isn't enough + { + auto ptr = cast(wchar*) malloc(wchar.sizeof * n); + scope(exit) free(ptr); + immutable n2 = GetCurrentDirectoryW(n, ptr); + cenforce(n2 && n2 < n, "getcwd"); + return toUTF8(ptr[0 .. n2]); + } } else { - auto dir = - new char[enforce(GetCurrentDirectoryA(0, null), "getcwd")]; - auto n = GetCurrentDirectoryA(dir.length, dir.ptr); - enforce(n && n < dir.length, "getcwd"); - return fromMBSz(cast(immutable)dir.ptr); + auto buffA = cast(char[]) staticBuff; + immutable n = cenforce(GetCurrentDirectoryA(buffA.length, buffA.ptr), "getcwd"); + // fromMBSz doesn't always produce a fresh string + if(n < buffA.length) + { + string res = fromMBSz(cast(immutable)buffA.ptr); + return res.ptr == buffA.ptr ? res.idup : res; + } + else //staticBuff isn't enough + { + auto ptr = cast(char*) malloc(char.sizeof * n); + scope(exit) free(ptr); + immutable n2 = GetCurrentDirectoryA(n, ptr); + cenforce(n2 && n2 < n, "getcwd"); + + string res = fromMBSz(cast(immutable)ptr); + return res.ptr == ptr ? res.idup : res; + } } }