Skip to content

Contributing back patches to 1DS #1266

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Contributing back patches to 1DS #1266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3210885
Contributing back patch
lakroy Apr 22, 2024
7b1826d
Applying ecs-proxy-fix.patch
lakroy Apr 22, 2024
85aea58
Merge branch 'main' into lakshmiroy/patch
mkoscumb Apr 22, 2024
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
108 changes: 107 additions & 1 deletion lib/http/HttpClient_WinInet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include <Wincrypt.h>
#include <WinInet.h>
#include <wincred.h>

#include <algorithm>
#include <memory>
Expand All @@ -22,6 +23,8 @@

namespace MAT_NS_BEGIN {

const std::string kProxyRegKeyPath = "Software\\Microsoft\\Windows\\CurrentVersion\\Internet Settings";
Copy link
Contributor

@mkoscumb mkoscumb Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 = ...


class WinInetRequestWrapper
{
protected:
Expand Down Expand Up @@ -136,6 +139,105 @@ class WinInetRequestWrapper
return true;
}

DWORD GetDWORDRegKey(HKEY hKey, const std::string& subKey, const std::string& value) {
Copy link
Contributor

@mkoscumb mkoscumb Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If these arguments are always known constants, don't do unnecessary heap allocations, use const char* rather than std::string

Copy link
Contributor

@mkoscumb mkoscumb Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

std::string_view would be ideal, but this repo is still not on C++17 :(

DWORD data;
DWORD dataSize = sizeof(data);
LONG res = RegGetValueA(hKey, subKey.c_str(), value.c_str(), RRF_RT_REG_DWORD, NULL, &data,
&dataSize);
if (res != ERROR_SUCCESS) {
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

How does a caller distinguish between a zero in the Registry value and an error?

}
return data;
}

std::string GetStringRegKey(HKEY hKey, const std::string& subKey, const std::string& value) {
DWORD stringSize;
LONG getSizeResult = RegGetValueA(hKey, subKey.c_str(), value.c_str(), RRF_RT_REG_SZ, NULL,
NULL, &stringSize);
if (getSizeResult != ERROR_SUCCESS) {
return "";
Copy link
Contributor

Choose a reason for hiding this comment

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

use the default string constructor, rather than the c-string constructor:
std::string{}

Copy link
Contributor

Choose a reason for hiding this comment

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

How does a caller distinguish between an empty string in the registry and an error?

}

std::string data;
data.resize(stringSize);
LONG getStringResult = RegGetValueA(hKey, subKey.c_str(), value.c_str(), RRF_RT_REG_SZ, NULL,
&data[0], &stringSize);
if (getStringResult != ERROR_SUCCESS) {
return "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

}

// Remove the null terminator from Win32 API. The std::string will already cover this.
data.resize(stringSize - 1);

return data;
}

bool ProxyAuthRequired() {
return GetDWORDRegKey(HKEY_CURRENT_USER, kProxyRegKeyPath, "ProxyEnable");
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't rely on implicit conversions.

GetDWORDRegKey(...) != 0

}

void SetProxyCredentials() {
// get proxy server from registry
std::string proxyServer = GetStringRegKey(HKEY_CURRENT_USER, kProxyRegKeyPath, "ProxyServer");
if (proxyServer.empty()) {
return;
}

std::wstring formattedProxyServer;

// If there is only one colon, then it is most likely of form IP:PORT and we just need
// to remove the port. If there are two colons, then it is most likely of form SCHEME:IP:PORT
int colonCount = 0;
for (const char& c : proxyServer) {
if (c == ':') {
colonCount++;
}
}

if (colonCount == 1) {
const auto colonPos = proxyServer.find_first_of(':');
auto portRemoved = proxyServer.substr(0, colonPos);
formattedProxyServer = std::wstring(portRemoved.begin(), portRemoved.end());
} else if (colonCount == 2) {
const auto firstColonPos = proxyServer.find_first_of(':');
auto schemeRemoved = proxyServer.substr(firstColonPos + 3);

const auto lastColonPos = schemeRemoved.find_last_of(':');
auto portAndSchemeRemoved = schemeRemoved.substr(0, lastColonPos);

formattedProxyServer = std::wstring(portAndSchemeRemoved.begin(),
portAndSchemeRemoved.end());
} else {
formattedProxyServer = std::wstring(proxyServer.begin(),
proxyServer.end());
}

// get proxy credentials from credential manager
PCREDENTIALW cred;
if (!::CredReadW(formattedProxyServer.c_str(), CRED_TYPE_GENERIC, 0, &cred)) {
return;
}
wchar_t* proxyUser = cred->UserName;
wchar_t* proxyPass = (wchar_t*)cred->CredentialBlob;
Copy link
Contributor

Choose a reason for hiding this comment

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

No c-style casts in modern code, use static_cast<wchar_t*> or reinterpret_cast<wchar_t*>


// set proxy credentials
if (proxyUser) {
::InternetSetOptionW(m_hWinInetRequest,
INTERNET_OPTION_PROXY_USERNAME,
static_cast<void*>(proxyUser),
static_cast<DWORD>(wcslen(proxyUser) + 1));
}

if (proxyPass) {
::InternetSetOptionW(m_hWinInetRequest,
INTERNET_OPTION_PROXY_PASSWORD,
static_cast<void*>(proxyPass),
static_cast<DWORD>(wcslen(proxyPass) + 1));
}

::CredFree(cred);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

The former is preferred as it's more resilient.

struct CredHolder final
{
    PCREDENTIALW cred;
    ~CredHolder() noexcept
    {
        ::CredFree(cred);
    }
};

}

// Asynchronously send HTTP request and invoke response callback.
// Ownership semantics: send(...) method self-destroys *this* upon
// receiving WinInet callback. There must be absolutely no methods
Expand Down Expand Up @@ -209,7 +311,7 @@ class WinInetRequestWrapper
PCSTR szAcceptTypes[] = {"*/*", NULL};
m_hWinInetRequest = ::HttpOpenRequestA(
m_hWinInetSession, m_request->m_method.c_str(), path, NULL, NULL, szAcceptTypes,
INTERNET_FLAG_KEEP_CONNECTION | INTERNET_FLAG_NO_AUTH | INTERNET_FLAG_NO_CACHE_WRITE |
INTERNET_FLAG_KEEP_CONNECTION | INTERNET_FLAG_NO_CACHE_WRITE |
INTERNET_FLAG_NO_COOKIES | INTERNET_FLAG_NO_UI | INTERNET_FLAG_PRAGMA_NOCACHE |
INTERNET_FLAG_RELOAD | (urlc.nScheme == INTERNET_SCHEME_HTTPS ? INTERNET_FLAG_SECURE : 0),
reinterpret_cast<DWORD_PTR>(this));
Expand Down Expand Up @@ -252,6 +354,10 @@ class WinInetRequestWrapper
return;
}

if (ProxyAuthRequired()) {
SetProxyCredentials();
}

// Try to send headers and request body to server
DispatchEvent(OnSending);
void *data = static_cast<void *>(m_request->m_body.data());
Expand Down
59 changes: 33 additions & 26 deletions lib/http/HttpResponseDecoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,18 @@

#include "HttpResponseDecoder.hpp"
#include "ILogManager.hpp"
#include <IHttpClient.hpp>
#include "utils/Utils.hpp"
#include <IHttpClient.hpp>
#include <algorithm>
#include <cassert>

#ifdef HAVE_MAT_JSONHPP
#include "json.hpp"
#endif

namespace MAT_NS_BEGIN {

HttpResponseDecoder::HttpResponseDecoder(ITelemetrySystem& system)
:
namespace MAT_NS_BEGIN
{
HttpResponseDecoder::HttpResponseDecoder(ITelemetrySystem& system) :
m_system(system)
{
}
Expand Down Expand Up @@ -46,17 +45,18 @@ namespace MAT_NS_BEGIN {
#endif

IHttpResponse const& response = *(ctx->httpResponse);
IHttpRequest & request = *(ctx->httpRequest);
IHttpRequest& request = *(ctx->httpRequest);

HttpRequestResult outcome = Abort;
auto result = response.GetResult();
switch (result) {
switch (result)
{
case HttpResult_OK:
if (response.GetStatusCode() == 200)
{
outcome = Accepted;
}
else if (response.GetStatusCode() >= 500 || response.GetStatusCode() == 408 || response.GetStatusCode() == 429)
else if (response.GetStatusCode() >= 500 || response.GetStatusCode() == 408 || response.GetStatusCode() == 429 || response.GetStatusCode() == 407)
{
outcome = RetryServer;
}
Expand All @@ -83,25 +83,28 @@ namespace MAT_NS_BEGIN {
processBody(response, outcome);
}

switch (outcome) {
case Accepted: {
switch (outcome)
{
case Accepted:
{
LOG_INFO("HTTP request %s finished after %d ms, events were successfully uploaded to the server",
response.GetId().c_str(), ctx->durationMs);
response.GetId().c_str(), ctx->durationMs);
{
DebugEvent evt;
evt.type = DebugEventType::EVT_HTTP_OK;
evt.param1 = response.GetStatusCode();
evt.data = static_cast<void *>(request.GetBody().data());
evt.data = static_cast<void*>(request.GetBody().data());
evt.size = request.GetBody().size();
DispatchEvent(evt);
}
eventsAccepted(ctx);
break;
}

case Rejected: {
case Rejected:
{
LOG_ERROR("HTTP request %s failed after %d ms, events were rejected by the server (%u) and will be all dropped",
response.GetId().c_str(), ctx->durationMs, response.GetStatusCode());
response.GetId().c_str(), ctx->durationMs, response.GetStatusCode());
std::string body(reinterpret_cast<char const*>(response.GetBody().data()), std::min<size_t>(response.GetBody().size(), 100));
LOG_TRACE("Server response: %s%s", body.c_str(), (response.GetBody().size() > body.size()) ? "..." : "");
{
Expand All @@ -112,21 +115,22 @@ namespace MAT_NS_BEGIN {
// This is to be addressed with ETW trace API that can send
// a detailed error context to ETW provider.
evt.param1 = response.GetStatusCode();
evt.data = static_cast<void *>(request.GetBody().data());
evt.data = static_cast<void*>(request.GetBody().data());
evt.size = request.GetBody().size();
DispatchEvent(evt);
eventsRejected(ctx);
}
break;
}

case Abort: {
case Abort:
{
LOG_WARN("HTTP request %s failed after %d ms, upload was aborted and events will be sent at a different time",
response.GetId().c_str(), ctx->durationMs);
response.GetId().c_str(), ctx->durationMs);
{
DebugEvent evt;
evt.type = DebugEventType::EVT_HTTP_FAILURE;
evt.param1 = 0; // response.GetStatusCode();
evt.param1 = 0; // response.GetStatusCode();
DispatchEvent(evt);
}
ctx->httpResponse = nullptr;
Expand All @@ -135,9 +139,10 @@ namespace MAT_NS_BEGIN {
break;
}

case RetryServer: {
case RetryServer:
{
LOG_WARN("HTTP request %s failed after %d ms, a temporary server error has occurred (%u) and events will be sent at a different time",
response.GetId().c_str(), ctx->durationMs, response.GetStatusCode());
response.GetId().c_str(), ctx->durationMs, response.GetStatusCode());
std::string body(reinterpret_cast<char const*>(response.GetBody().data()), std::min<size_t>(response.GetBody().size(), 100));
LOG_TRACE("Server response: %s%s", body.c_str(), (response.GetBody().size() > body.size()) ? "..." : "");
{
Expand All @@ -150,9 +155,10 @@ namespace MAT_NS_BEGIN {
break;
}

case RetryNetwork: {
case RetryNetwork:
{
LOG_WARN("HTTP request %s failed after %d ms, a network error has occurred and events will be sent at a different time",
response.GetId().c_str(), ctx->durationMs);
response.GetId().c_str(), ctx->durationMs);
{
DebugEvent evt;
evt.type = DebugEventType::EVT_HTTP_FAILURE;
Expand All @@ -165,7 +171,7 @@ namespace MAT_NS_BEGIN {
}
}

void HttpResponseDecoder::processBody(IHttpResponse const& response, HttpRequestResult & result)
void HttpResponseDecoder::processBody(IHttpResponse const& response, HttpRequestResult& result)
{
#ifdef HAVE_MAT_JSONHPP
// TODO: [MG] - parse HTTP response without json.hpp library
Expand Down Expand Up @@ -227,7 +233,8 @@ namespace MAT_NS_BEGIN {
if (result != Rejected)
{
LOG_TRACE("HTTP response: accepted=%d rejected=%d", accepted, rejected);
} else
}
else
{
LOG_TRACE("HTTP response: all rejected");
}
Expand All @@ -242,5 +249,5 @@ namespace MAT_NS_BEGIN {
#endif
}

} MAT_NS_END

}
MAT_NS_END
5 changes: 5 additions & 0 deletions lib/include/public/IECSClient.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "ILogger.hpp"

#include <string>
#include <unordered_set>
#include <vector>
#include <map>

Expand Down Expand Up @@ -51,6 +52,10 @@ namespace Microsoft {

// [optional] enabled ECS telemetry
bool enableECSClientTelemetry = false;

// [optional] Mandatory agents list. If not present in response, the payload
// is determined as bad
std::unordered_set<std::string> mandatoryAgents;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you miss adding any file, as mandatoryAgents is not used anywhere?

Copy link
Author

Choose a reason for hiding this comment

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

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.

};

/// <summary>
Expand Down
Loading