Breakwell commented on a change in pull request #2083:
URL: https://github.com/apache/thrift/pull/2083#discussion_r432406861
##########
File path: lib/cpp/src/thrift/transport/TPipeServer.cpp
##########
@@ -323,73 +344,47 @@ void TPipeServer::close() {
impl_.reset();
}
-bool TNamedPipeServer::createNamedPipe(const TAutoCrit & /*lockProof*/) {
-
- // Windows - set security to allow non-elevated apps
- // to access pipes created by elevated apps.
- SID_IDENTIFIER_AUTHORITY SIDAuthWorld = SECURITY_WORLD_SID_AUTHORITY;
- PSID everyone_sid = NULL;
- AllocateAndInitializeSid(
- &SIDAuthWorld, 1, SECURITY_WORLD_RID, 0, 0, 0, 0, 0, 0, 0,
&everyone_sid);
-
- EXPLICIT_ACCESS ea;
- ZeroMemory(&ea, sizeof(EXPLICIT_ACCESS));
- ea.grfAccessPermissions = SPECIFIC_RIGHTS_ALL | STANDARD_RIGHTS_ALL;
- ea.grfAccessMode = SET_ACCESS;
- ea.grfInheritance = NO_INHERITANCE;
- ea.Trustee.TrusteeForm = TRUSTEE_IS_SID;
- ea.Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
- ea.Trustee.ptstrName = static_cast<LPTSTR>(everyone_sid);
-
- PACL acl = NULL;
- SetEntriesInAcl(1, &ea, NULL, &acl);
-
- PSECURITY_DESCRIPTOR sd = (PSECURITY_DESCRIPTOR)LocalAlloc(LPTR,
SECURITY_DESCRIPTOR_MIN_LENGTH);
- if (!InitializeSecurityDescriptor(sd, SECURITY_DESCRIPTOR_REVISION)) {
- auto lastError = GetLastError();
- LocalFree(sd);
- LocalFree(acl);
- GlobalOutput.perror("TPipeServer::InitializeSecurityDescriptor() GLE=",
lastError);
- throw TTransportException(TTransportException::NOT_OPEN,
"InitializeSecurityDescriptor() failed",
- lastError);
- }
- if (!SetSecurityDescriptorDacl(sd, TRUE, acl, FALSE)) {
- auto lastError = GetLastError();
- LocalFree(sd);
- LocalFree(acl);
- GlobalOutput.perror("TPipeServer::SetSecurityDescriptorDacl() GLE=",
lastError);
- throw TTransportException(TTransportException::NOT_OPEN,
- "SetSecurityDescriptorDacl() failed", lastError);
+bool TNamedPipeServer::createNamedPipe(const TAutoCrit& /*lockProof*/) {
+
+ PSECURITY_DESCRIPTOR psd = NULL;
+ ULONG size = 0;
+
+ if
(!ConvertStringSecurityDescriptorToSecurityDescriptorA(securityDescriptor_.c_str(),
+ SDDL_REVISION_1,
&psd, &size)) {
+ DWORD lastError = GetLastError();
+
GlobalOutput.perror("TPipeServer::ConvertStringSecurityDescriptorToSecurityDescriptorA()
GLE=",
+ lastError);
+ throw TTransportException(
+ TTransportException::NOT_OPEN,
+ "TPipeServer::ConvertStringSecurityDescriptorToSecurityDescriptorA()
failed", lastError);
}
SECURITY_ATTRIBUTES sa;
sa.nLength = sizeof(SECURITY_ATTRIBUTES);
- sa.lpSecurityDescriptor = sd;
+ sa.lpSecurityDescriptor = psd;
sa.bInheritHandle = FALSE;
// Create an instance of the named pipe
- TAutoHandle hPipe(CreateNamedPipeA(pipename_.c_str(), // pipe name
- PIPE_ACCESS_DUPLEX | // read/write access
- FILE_FLAG_OVERLAPPED, // async mode
- PIPE_TYPE_BYTE | // byte type pipe
- PIPE_READMODE_BYTE, // byte read mode
- maxconns_, // max. instances
- bufsize_, // output buffer
size
- bufsize_, // input buffer size
- 0, // client time-out
- &sa)); // security
attributes
+ TAutoHandle hPipe(CreateNamedPipeA(pipename_.c_str(), // pipe name
+ PIPE_ACCESS_DUPLEX | // read/write
access
+ FILE_FLAG_OVERLAPPED, // async mode
+ PIPE_TYPE_BYTE | // byte type
pipe
+ PIPE_READMODE_BYTE, // byte read
mode
+ maxconns_, // max.
instances
+ bufsize_, // output
buffer size
+ bufsize_, // input buffer
size
+ 0, // client
time-out
+ &sa)); // security
attributes
auto lastError = GetLastError();
- LocalFree(sd);
- LocalFree(acl);
- FreeSid(everyone_sid);
+ if (psd)
+ LocalFree(psd);
Review comment:
I feel it's a bit overkill to add a RAII considering we only free once.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
