Skip to content

Clean up some code in ANCM #49718

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 31, 2023
Merged

Clean up some code in ANCM #49718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4d8a80
Misc code cleanup in ANCM.
adityamandaleeka Jul 29, 2023
6c49127
Missed files.
adityamandaleeka Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Servers/IIS/AspNetCoreModuleV2/AspNetCore/globalmodule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ ASPNET_CORE_GLOBAL_MODULE::OnGlobalConfigurationChange(
// We need this duplicate setting because the global module is unable to read the app settings disallowRotationOnConfigChange value.
if (m_pApplicationManager && m_pApplicationManager->ShouldRecycleOnConfigChange())
{
m_pApplicationManager->RecycleApplicationFromManager(pwszChangePath);
m_pApplicationManager->RecycleApplicationFromManager(pwszChangePath);
}
}

Expand Down
7 changes: 0 additions & 7 deletions src/Servers/IIS/AspNetCoreModuleV2/CommonLib/HandleWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,6 @@ struct NullHandleTraits
static void Close(HANDLE handle) noexcept { CloseHandle(handle); }
};

struct FindFileHandleTraits
{
using HandleType = HANDLE;
static constexpr HANDLE DefaultHandle = nullptr;
static void Close(HANDLE handle) noexcept { FindClose(handle); }
};

struct ModuleHandleTraits
{
using HandleType = HMODULE;
Expand Down
21 changes: 9 additions & 12 deletions src/Servers/IIS/AspNetCoreModuleV2/CommonLib/debugutil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,21 +357,18 @@ DebugPrintW(

OutputDebugString( strOutput.QueryStr() );

if (IsEnabled(ASPNETCORE_DEBUG_FLAG_CONSOLE) || g_logFile != INVALID_HANDLE_VALUE)
if (IsEnabled(ASPNETCORE_DEBUG_FLAG_CONSOLE))
{
if (IsEnabled(ASPNETCORE_DEBUG_FLAG_CONSOLE))
{
WriteFileEncoded(GetConsoleOutputCP(), g_stdOutHandle, strOutput.QueryStr());
}
WriteFileEncoded(GetConsoleOutputCP(), g_stdOutHandle, strOutput.QueryStr());
}

if (g_logFile != INVALID_HANDLE_VALUE)
{
SRWExclusiveLock lock(g_logFileLock);
if (g_logFile != INVALID_HANDLE_VALUE)
{
SRWExclusiveLock lock(g_logFileLock);

SetFilePointer(g_logFile, 0, nullptr, FILE_END);
WriteFileEncoded(CP_UTF8, g_logFile, strOutput.QueryStr());
FlushFileBuffers(g_logFile);
}
SetFilePointer(g_logFile, 0, nullptr, FILE_END);
WriteFileEncoded(CP_UTF8, g_logFile, strOutput.QueryStr());
FlushFileBuffers(g_logFile);
}

if (IsEnabled(ASPNETCORE_DEBUG_FLAG_EVENTLOG))
Expand Down
105 changes: 43 additions & 62 deletions src/Servers/IIS/AspNetCoreModuleV2/CommonLib/file_utility.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,30 +11,23 @@
HRESULT
FILE_UTILITY::IsPathUnc(
__in LPCWSTR pszPath,
__out BOOL * pfIsUnc
__out bool* pfIsUnc
)
{
HRESULT hr = S_OK;
STRU strTempPath;

if ( pszPath == NULL || pfIsUnc == NULL )
if (pszPath == nullptr || pfIsUnc == nullptr)
{
hr = E_INVALIDARG;
goto Finished;
return E_INVALIDARG;
}

hr = MakePathCanonicalizationProof( (LPWSTR) pszPath, &strTempPath );
if ( FAILED(hr) )
STRU strTempPath;
HRESULT hr = MakePathCanonicalizationProof(pszPath, &strTempPath );
if (FAILED(hr))
{
goto Finished;
return hr;
}

//
// MakePathCanonicalizationProof will map \\?\UNC, \\.\UNC and \\ to \\?\UNC
//
(*pfIsUnc) = ( _wcsnicmp( strTempPath.QueryStr(), L"\\\\?\\UNC\\", 8 /* sizeof \\?\UNC\ */) == 0 );

Finished:
// MakePathCanonicalizationProof will map \\?\UNC, \\.\UNC and \\ to \\?\UNC (which is 8 characters)
*pfIsUnc = (_wcsnicmp(strTempPath.QueryStr(), L"\\\\?\\UNC\\", 8) == 0);

return hr;
}
Expand Down Expand Up @@ -103,69 +96,57 @@ FILE_UTILITY::ConvertPathToFullPath(
return hr;
}

// Ensures that the specified directory path exists, creating it if necessary. If any part of the directory
// creation fails and the directory does not already exist, it returns an error obtained from GetLastError.
// If all directories are successfully created or already exist, it returns S_OK.
HRESULT
FILE_UTILITY::EnsureDirectoryPathExist(
_In_ LPCWSTR pszPath
FILE_UTILITY::EnsureDirectoryPathExists(
Copy link
Member

Choose a reason for hiding this comment

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

Need to change the header and caller

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I used the VS refactor thing and forgot to save the other modified files 😆

_In_ const LPCWSTR pszPath
)
{
HRESULT hr = S_OK;
STRU struPath;
DWORD dwPosition = 0;
BOOL fDone = FALSE;
BOOL fUnc = FALSE;

STRU struPath;
struPath.Copy(pszPath);
hr = IsPathUnc(pszPath, &fUnc);

bool isUnc = false;
HRESULT hr = IsPathUnc(pszPath, &isUnc);
if (FAILED(hr))
{
goto Finished;
}
if (fUnc)
{
// "\\?\UNC\"
dwPosition = 8;
return hr;
}
else if (struPath.IndexOf(L'?', 0) != -1)
{
// sceanrio "\\?\"
dwPosition = 4;
}
while (!fDone)

// Initialize position based on the type of the path.
DWORD position = isUnc ? 8 // Skip "\\?\UNC\"
: (struPath.IndexOf(L'?', 0) != -1) ? 4 // Skip "\\?\"
: 0; // Skip nothing

// Traverse the path string, creating directories as we go.
while (true)
{
dwPosition = struPath.IndexOf(L'\\', dwPosition + 1);
if (dwPosition == -1)
{
// not found '/'
fDone = TRUE;
goto Finished;
}
else if (dwPosition ==0)
position = struPath.IndexOf(L'\\', position + 1);
if (position == -1)
{
hr = ERROR_INTERNAL_ERROR;
goto Finished;
// Didn't find a path separator, so we're done.
return S_OK;
}
else if (struPath.QueryStr()[dwPosition-1] == L':')

// position is >= 1 here since we started searching at position + 1
if (struPath.QueryStr()[position - 1] == L':')
Copy link
Member

Choose a reason for hiding this comment

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

You removed the position == 0 condition so this one can buffer underrun now

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it because it can never be 0. Can it? Even in the case where we start at 0, the STRU::IndexOf is given the start offset of position+1 to search from, and it will either return -1 (if not found) or a position >=1.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I guess. Probably add a comment then since it wasn't obvious from first glance

{
// skip volume case
// Skip the volume case
continue;
}
else
{
struPath.QueryStr()[dwPosition] = L'\0';
}

if (!CreateDirectory(struPath.QueryStr(), NULL) &&
ERROR_ALREADY_EXISTS != GetLastError())
// Temporarily terminate the string at the current path separator
struPath.QueryStr()[position] = L'\0';
if (!CreateDirectory(struPath.QueryStr(), nullptr) && GetLastError() != ERROR_ALREADY_EXISTS)
{
hr = HRESULT_FROM_WIN32(GetLastError());
fDone = TRUE;
goto Finished;
// Unable to create directory and it doesn't already exist
return HRESULT_FROM_WIN32(GetLastError());
}
struPath.QueryStr()[dwPosition] = L'\\';
}

Finished:
return hr;
// Restore the path separator
struPath.QueryStr()[position] = L'\\';
}
}

std::string FILE_UTILITY::GetHtml(HMODULE module, int page, USHORT statusCode, USHORT subStatusCode, const std::string& specificReasonPhrase, const std::string& solution)
Expand Down
4 changes: 2 additions & 2 deletions src/Servers/IIS/AspNetCoreModuleV2/CommonLib/file_utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class FILE_UTILITY

static
HRESULT
EnsureDirectoryPathExist(
EnsureDirectoryPathExists(
_In_ LPCWSTR pszPath
);

Expand All @@ -40,7 +40,7 @@ class FILE_UTILITY
HRESULT
IsPathUnc(
__in LPCWSTR pszPath,
__out BOOL * pfIsUnc
__out bool * pfIsUnc
);
};

2 changes: 1 addition & 1 deletion src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/serverprocess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1086,7 +1086,7 @@ SERVER_PROCESS::SetupStdHandles(
goto Finished;
}

hr = FILE_UTILITY::EnsureDirectoryPathExist(struPath.QueryStr());
hr = FILE_UTILITY::EnsureDirectoryPathExists(struPath.QueryStr());
if (FAILED_LOG(hr))
{
goto Finished;
Expand Down