Contributing back patches to 1DS #1266
Conversation
|
|
||
| // [optional] Mandatory agents list. If not present in response, the payload | ||
| // is determined as bad | ||
| std::unordered_set<std::string> mandatoryAgents; |
There was a problem hiding this comment.
Did you miss adding any file, as mandatoryAgents is not used anywhere?
There was a problem hiding this comment.
It will be used in the EXPCommonClient.cpp file in this PR - Pull 230 - Contributing back patches to 1DS modules. I think this PR will have to be merged first before we merged the private one.
|
Thanks @lakshmir124, can you please add more details to the PR description? |
lakshmir124
changed the title
| @@ -22,6 +23,8 @@ | |||
|
|
|||
| namespace MAT_NS_BEGIN { | |||
|
|
|||
| const std::string kProxyRegKeyPath = "Software\\Microsoft\\Windows\\CurrentVersion\\Internet Settings"; | |||
There was a problem hiding this comment.
Don't create namespace scoped STL containers as these cause heap allocations which are never released until the containing binary is unloaded, which violates Pay For Play.
static const char* kProxyRegKeyPath = ...
| LONG getSizeResult = RegGetValueA(hKey, subKey.c_str(), value.c_str(), RRF_RT_REG_SZ, NULL, | ||
| NULL, &stringSize); | ||
| if (getSizeResult != ERROR_SUCCESS) { | ||
| return ""; |
There was a problem hiding this comment.
use the default string constructor, rather than the c-string constructor:
std::string{}
| LONG getStringResult = RegGetValueA(hKey, subKey.c_str(), value.c_str(), RRF_RT_REG_SZ, NULL, | ||
| &data[0], &stringSize); | ||
| if (getStringResult != ERROR_SUCCESS) { | ||
| return ""; |
| LONG res = RegGetValueA(hKey, subKey.c_str(), value.c_str(), RRF_RT_REG_DWORD, NULL, &data, | ||
| &dataSize); | ||
| if (res != ERROR_SUCCESS) { | ||
| return 0; |
There was a problem hiding this comment.
How does a caller distinguish between a zero in the Registry value and an error?
| LONG getSizeResult = RegGetValueA(hKey, subKey.c_str(), value.c_str(), RRF_RT_REG_SZ, NULL, | ||
| NULL, &stringSize); | ||
| if (getSizeResult != ERROR_SUCCESS) { | ||
| return ""; |
There was a problem hiding this comment.
How does a caller distinguish between an empty string in the registry and an error?
| @@ -136,6 +139,105 @@ class WinInetRequestWrapper | |||
| return true; | |||
| } | |||
|
|
|||
| DWORD GetDWORDRegKey(HKEY hKey, const std::string& subKey, const std::string& value) { | |||
There was a problem hiding this comment.
If these arguments are always known constants, don't do unnecessary heap allocations, use const char* rather than std::string
There was a problem hiding this comment.
std::string_view would be ideal, but this repo is still not on C++17 :(
| } | ||
|
|
||
| bool ProxyAuthRequired() { | ||
| return GetDWORDRegKey(HKEY_CURRENT_USER, kProxyRegKeyPath, "ProxyEnable"); |
There was a problem hiding this comment.
Don't rely on implicit conversions.
GetDWORDRegKey(...) != 0
| return; | ||
| } | ||
| wchar_t* proxyUser = cred->UserName; | ||
| wchar_t* proxyPass = (wchar_t*)cred->CredentialBlob; |
There was a problem hiding this comment.
No c-style casts in modern code, use static_cast<wchar_t*> or reinterpret_cast<wchar_t*>
| static_cast<DWORD>(wcslen(proxyPass) + 1)); | ||
| } | ||
|
|
||
| ::CredFree(cred); |
There was a problem hiding this comment.
You should either wrap PCREDENTIALW in a helper type whose destructor calls ::CredFree, or refactor 216-238 into a helper function which is marked noexcept. That way if at any point (including if someone adds new code which can raise exceptions) an exception is raised there's no chance cred is leaked.
There was a problem hiding this comment.
The former is preferred as it's more resilient.
struct CredHolder final
{
PCREDENTIALW cred;
~CredHolder() noexcept
{
::CredFree(cred);
}
};
This PR contributes back cpp-client-telemetry related patches used in the teams-client-native-shell repo.