Use SString type - PathString for path allocations in binder. #1589

Priya91 · 2015-09-18T01:17:56Z

jkotas · 2015-09-18T01:26:41Z

src/binder/cdebuglog.cpp

@@ -65,8 +65,11 @@ namespace BINDER_SPACE
                IF_FAIL_GO(HRESULT_FROM_WIN32(ERROR_BUFFER_OVERFLOW));
            }

-            IF_FAIL_GO(StringCbCopy(szPath, sizeof(szPath), pszName));
-
+            WCHAR * szPath = szPathString.OpenUnicodeBuffer(MAX_LONGPATH);


The point of using variable size optimized buffer class is that we should be allocating only what is needed whenever possible. We know exactly on how much we need to allocate here - so we should only allocate as much.

Priya91 · 2015-09-22T08:43:23Z

@jkotas @janvorli @JeremyKuhne : Added commit to replace allocations that are in jeremy's linux longpath change.

jkotas · 2015-09-22T10:56:37Z

src/inc/sstring.h

@@ -1006,6 +1006,9 @@ typedef InlineSString<512> StackSString;
 // be needed is small and it's preferable not to take up the stack space.
 typedef InlineSString<32>  SmallStackSString;

+// To be used specifically for path strings.
+typedef InlineSString<512> PathString;


I would make this 256 (=existing MAX_PATH). Also, I would make the size much smaller in debug builds to make sure that we exercise the buffer allocation path.

#ifdef _DEBUG // Make the size smaller in debug builds to exercise the buffer allocation path typedef InlineSString<32> PathString; #else typedef InlineSString<256> PathString; #endif

jkotas · 2015-09-22T16:15:28Z

@Priya91 I went through all changes and left comments.

Priya91 · 2015-09-24T00:14:40Z

@jkotas I've submitted a new commit with the PR feedback, please review for merge. Thanks!

jkotas · 2015-09-24T00:30:55Z

src/dlls/mscoree/mscoree.cpp

+    WCHAR * pPath = new (nothrow) WCHAR[MAX_LONGPATH];
+    if (pPath == NULL)
+    {
+        WCHAR pPathStack[MAX_LONGPATH];


The error handling here should be:

SetLastError(ERROR_NOT_ENOUGH_MEMORY); return FALSE;

BTW: pPathStack in your original code is guaranteed to be allocated on stack only by the closing } of the scope - pPath would be a dangling pointer.

I am not sure I understand the purpose of this construct. The pPathStack will be allocated always on the stack, so why do we allocate the pPath at all? And for compilers that would reuse the frame space of the pPathStack after it goes out of scope at the end of the block, this is even dangerous.

Yup, I just realised that. Didn't know how to handle NULL here.

Priya91 · 2015-09-24T23:04:14Z

@jkotas @janvorli @JeremyKuhne rebased the feedback commits into a single commit. When this passes CI run, is it good to merge?

janvorli · 2015-09-24T23:45:32Z

src/binder/utils.cpp

-            BINDER_LOG_STRING(W("combined path"), tempResultPath);
-        }
-        else
+        PathString tempResultPath;


We could combine it directly into the combinedPath instead of creating the tempResultPath.

Priya91 · 2015-09-25T01:21:37Z

"d:\j\workspace\dotnet_coreclr_windows_debug_prtest\bin\obj\Windows_NT.x64.Debug\install.vcxproj" (default target) (1) ->
00:39:40        "D:\j\workspace\dotnet_coreclr_windows_debug_prtest\bin\obj\Windows_NT.x64.Debug\ALL_BUILD.vcxproj" (default target) (2) ->
00:39:40        "D:\j\workspace\dotnet_coreclr_windows_debug_prtest\bin\obj\Windows_NT.x64.Debug\src\dlls\mscoree\coreclr\coreclr.vcxproj" (default target) (20) ->
00:39:40        "D:\j\workspace\dotnet_coreclr_windows_debug_prtest\bin\obj\Windows_NT.x64.Debug\src\binder\v3binder\v3binder.vcxproj" (default target) (56) ->
00:39:40          C:\Program Files (x86)\MSBuild\Microsoft\VisualStudio\v14.0\CodeAnalysis\Microsoft.CodeAnalysis.targets(214,5): error MSB4175: The task factory "CodeTaskFactory" could not be loaded from the assembly "C:\Program Files (x86)\MSBuild\14.0\bin\Microsoft.Build.Tasks.Core.dll". Could not find file 'C:\Users\dotnet-bot\AppData\Local\Temp\unr0z0yu.dll'. [D:\j\workspace\dotnet_coreclr_windows_debug_prtest\bin\obj\Windows_NT.x64.Debug\src\binder\v3binder\v3binder.vcxproj]

@dotnet-bot test this please.

jkotas · 2015-09-25T02:50:10Z

src/dlls/mscoree/mscoree.cpp

+    WCHAR * pPath = new (nothrow) WCHAR[MAX_LONGPATH];
+    if (pPath == NULL)
+    {
+        return HRESULT_FROM_WIN32(ERROR_NOT_ENOUGH_MEMORY);


This should be return FALSE; - this function does not return HRESULT.

jkotas · 2015-09-25T02:58:58Z

LGTM othewise

Respond to PR feedback. Fix test failures in mscoree.

Use SString type - PathString for path allocations in binder.

Use SString type - PathString for path allocations in binder. Commit migrated from dotnet/coreclr@bdc5bce

dotnet-bot added the 2 - In Progress label Sep 18, 2015

dnfclas added the cla-already-signed label Sep 18, 2015

jkotas reviewed Sep 18, 2015
View reviewed changes

Use SString type for path allocations in binder.

b8282e0

Priya91 force-pushed the pathstring branch from f2cb82a to b8282e0 Compare September 21, 2015 21:35

jkotas reviewed Sep 22, 2015
View reviewed changes

Priya91 force-pushed the pathstring branch 3 times, most recently from 9e183c3 to a2b8a58 Compare September 24, 2015 00:13

Priya91 force-pushed the pathstring branch from a2b8a58 to a9cd7e2 Compare September 24, 2015 00:26

jkotas reviewed Sep 24, 2015
View reviewed changes

Priya91 force-pushed the pathstring branch 2 times, most recently from f0fae35 to e5789d9 Compare September 24, 2015 23:02

janvorli reviewed Sep 24, 2015
View reviewed changes

Priya91 force-pushed the pathstring branch from e5789d9 to 6be46ec Compare September 25, 2015 00:33

jkotas reviewed Sep 25, 2015
View reviewed changes

Use PathString type to allocate large path strings on linux scenario.

965e5d6

Respond to PR feedback. Fix test failures in mscoree.

Priya91 force-pushed the pathstring branch from 6be46ec to 965e5d6 Compare September 25, 2015 20:50

Priya91 added a commit that referenced this pull request Sep 28, 2015

Merge pull request #1589 from Priya91/pathstring

bdc5bce

Use SString type - PathString for path allocations in binder.

Priya91 merged commit bdc5bce into dotnet:master Sep 28, 2015

dotnet-bot removed the 2 - In Progress label Sep 28, 2015

Priya91 deleted the pathstring branch September 28, 2015 22:56

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#1589 from Priya91/pathstring

6ce063f

Use SString type - PathString for path allocations in binder. Commit migrated from dotnet/coreclr@bdc5bce

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use SString type - PathString for path allocations in binder. #1589

Use SString type - PathString for path allocations in binder. #1589

Uh oh!

Priya91 commented Sep 18, 2015

Uh oh!

jkotas Sep 18, 2015

Uh oh!

Priya91 commented Sep 22, 2015

Uh oh!

jkotas Sep 22, 2015

Uh oh!

jkotas commented Sep 22, 2015

Uh oh!

Priya91 commented Sep 24, 2015

Uh oh!

jkotas Sep 24, 2015

Uh oh!

jkotas Sep 24, 2015

Uh oh!

janvorli Sep 24, 2015

Uh oh!

Priya91 Sep 24, 2015

Uh oh!

Priya91 commented Sep 24, 2015

Uh oh!

janvorli Sep 24, 2015

Uh oh!

Priya91 commented Sep 25, 2015

Uh oh!

jkotas Sep 25, 2015

Uh oh!

jkotas commented Sep 25, 2015

Uh oh!

Uh oh!

Use SString type - PathString for path allocations in binder. #1589

Use SString type - PathString for path allocations in binder. #1589

Uh oh!

Conversation

Priya91 commented Sep 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Priya91 commented Sep 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Sep 22, 2015

Uh oh!

Priya91 commented Sep 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Priya91 commented Sep 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Priya91 commented Sep 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Sep 25, 2015

Uh oh!

Uh oh!