Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(249)

Issue 5792731695677440: Fix named pipe security on Windows 7 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 5 months ago by Oleksandr
Modified:
5 years, 3 months ago
Reviewers:
Eric, sergei, Felix Dahlke
Visibility:
Public.

Description

Fix named pipe security on Windows 7

Patch Set 1 #

Total comments: 10

Patch Set 2 : Refactor to eliminate the corrupt ACL bug. Address comments. #

Total comments: 15

Patch Set 3 : Use custom free function. Make sure sure SACL is released correctly as well. #

Total comments: 12

Patch Set 4 : Nits addressed #

Patch Set 5 : Hide the spelling fail #

Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -19 lines) Patch
M src/shared/Communication.cpp View 1 2 3 4 7 chunks +52 lines, -19 lines 0 comments Download

Messages

Total messages: 21
Oleksandr
5 years, 5 months ago (2014-05-13 12:26:18 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Communication.cpp#newcode105 src/shared/Communication.cpp:105: std::tr1::shared_ptr<ACL> sharedAcl(static_cast<ACL*>(acl), LocalFree); // Just to simplify cleanup If ...
5 years, 5 months ago (2014-05-15 07:36:18 UTC) #2
Felix Dahlke
On 2014/05/15 07:36:18, Wladimir Palant wrote: > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Communication.cpp > File src/shared/Communication.cpp (right): > > http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Communication.cpp#newcode105 ...
5 years, 3 months ago (2014-06-24 15:17:06 UTC) #3
Eric
http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Communication.cpp#newcode58 src/shared/Communication.cpp:58: std::auto_ptr<SECURITY_DESCRIPTOR> CreateSecurityDescriptor(PSID logonSid) We can use unique_ptr here. VS2012 ...
5 years, 3 months ago (2014-06-25 16:13:00 UTC) #4
Felix Dahlke
http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Communication.cpp#newcode58 src/shared/Communication.cpp:58: std::auto_ptr<SECURITY_DESCRIPTOR> CreateSecurityDescriptor(PSID logonSid) On 2014/06/25 16:13:00, Eric wrote: > ...
5 years, 3 months ago (2014-06-25 16:17:13 UTC) #5
Oleksandr
5 years, 3 months ago (2014-07-02 09:18:27 UTC) #6
Felix Dahlke
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp#newcode101 src/shared/Communication.cpp:101: // acl has to be released after this I ...
5 years, 3 months ago (2014-07-03 13:38:07 UTC) #7
Oleksandr
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp#newcode101 src/shared/Communication.cpp:101: // acl has to be released after this 1. ...
5 years, 3 months ago (2014-07-03 13:51:49 UTC) #8
Felix Dahlke
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp#newcode101 src/shared/Communication.cpp:101: // acl has to be released after this On ...
5 years, 3 months ago (2014-07-03 13:56:33 UTC) #9
Oleksandr
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Utils.cpp File src/shared/Utils.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Utils.cpp#newcode19 src/shared/Utils.cpp:19: std::unique_ptr<OSVERSIONINFOEX> GetWindowsVersion() http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Utils.cpp#newcode28 and http://codereview.adblockplus.org/5792731695677440/diff/5629499534213120/src/shared/Utils.cpp#newcode34 actually On 2014/07/03 13:56:33, ...
5 years, 3 months ago (2014-07-03 14:00:42 UTC) #10
Felix Dahlke
On 2014/07/03 14:00:42, Oleksandr wrote: > http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Utils.cpp > File src/shared/Utils.cpp (right): > > http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Utils.cpp#newcode19 > ...
5 years, 3 months ago (2014-07-03 14:08:16 UTC) #11
sergei
Here is yet one thing, sacl from dummy security descriptor is also removed too early, ...
5 years, 3 months ago (2014-07-11 13:39:49 UTC) #12
Oleksandr
Comments addressed http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp#newcode105 src/shared/Communication.cpp:105: // This only references the acl, not ...
5 years, 3 months ago (2014-07-15 14:16:55 UTC) #13
sergei
I personally don't like the idea that the caller has to call cleaning functions. I ...
5 years, 3 months ago (2014-07-15 15:30:50 UTC) #14
Oleksandr
You do have a point, and the solution you propose does look more elegant, however ...
5 years, 3 months ago (2014-07-16 12:23:52 UTC) #15
Felix Dahlke
On 2014/07/16 12:23:52, Oleksandr wrote: > You do have a point, and the solution you ...
5 years, 3 months ago (2014-07-16 14:19:45 UTC) #16
Felix Dahlke
http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5709068098338816/src/shared/Communication.cpp#newcode105 src/shared/Communication.cpp:105: // This only references the acl, not copies it ...
5 years, 3 months ago (2014-07-16 14:40:02 UTC) #17
Oleksandr
http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/shared/Communication.cpp#newcode111 src/shared/Communication.cpp:111: // Create a dummy security descriptor with low integrirty ...
5 years, 3 months ago (2014-07-17 17:32:48 UTC) #18
Felix Dahlke
Just one remaining :) http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/shared/Communication.cpp#newcode111 src/shared/Communication.cpp:111: // Create a dummy security ...
5 years, 3 months ago (2014-07-18 12:40:56 UTC) #19
Oleksandr
http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/5792731695677440/diff/5684049913839616/src/shared/Communication.cpp#newcode111 src/shared/Communication.cpp:111: // Create a dummy security descriptor with low integrirty ...
5 years, 3 months ago (2014-07-18 12:48:33 UTC) #20
Felix Dahlke
5 years, 3 months ago (2014-07-18 12:50:46 UTC) #21
LGTM!!11!!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5