Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562473002:
Fixing SecurityKeySocket unit test issues and cleanup (Closed)
|
Created:
4 years ago by joedow Modified:
4 years ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, chromoting-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixing SecurityKeySocket unit test issues
The low-level security key unit tests had a few problems that I am
fixing in this CL:
1.) Several unit tests didn't actually test anything
2.) The other unit tests did not have enough validation
There is also some minor renaming and trimming to get more code to fit
on a single line instead of spanning mulitple lines.
BUG=630659
Committed: https://crrev.com/865c534bef93f875acf0073efcbbf79e201339fd
Cr-Commit-Position: refs/heads/master@{#438408}
Patch Set 1 #Patch Set 2 : Fixing some comments and unnecessary headers #
Total comments: 6
Patch Set 3 : Addressing Feedback #Patch Set 4 : Added one more validation to the timeout unit test #
Total comments: 6
Patch Set 5 : Addressing CR Feedback #2 #
Messages
Total messages: 36 (28 generated)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
joedow@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL!
I'm not sure how this change is related to the issue in the bug. The bug shows -101 error code when trying to write an error message. -101 = NET_CONNECTION_RESET, which likely means that the process on the other end of connection has died. Also I'm not sure it's worth worrying about the finishing error message send when it cannot be completed after the first send() call. Normally there should be space for the message in the socket buffer, so it's very unlikely that the first write doesn't write the whole message. https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... File remoting/host/security_key/security_key_auth_handler_posix.cc (right): https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix.cc:57: void DeleteSocket(std::unique_ptr<remoting::SecurityKeySocket> socket) {} base::DeletePointer() can be used instead of this function https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix.cc:301: base::Bind(&DeleteSocket, base::Passed(&iter->second))); I don't think there is anything that guarantees that we will ever succeed to send the error message, so it's possible that the the socket will never be deleted, so I'm not sure that just releasing it here is the right thing to do. Also it allows for other subtle bugs, e.g. can we be sure that SecurityKeySocket will not call timeout callback later, after SecurityKeyAuthHandlerPosix is destroyed? https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... File remoting/host/security_key/security_key_socket.cc (right): https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_socket.cc:64: response_written_callback_ = response_written_callback; If SendResponse() is called before previous response is sent then the callback for the previous response will never be called. In this CL the callback is only used for the error in the end, so maybe we shouldn't allow response_written_callback only for SsendSshError(). I.e. move this line to SendSshError() and add DCHECK(!response_written_callback_).
Description was changed from ========== Fixing SecurityKeySocket SSH error problem and unit test issues There were a couple of issue addressed in this CL: 1.) SSH errors weren't always sent correctly, the write is async so it is possible they are not completed before the socket is destoryed 2.) Several unit tests didn't actually test anything 3.) The other unit tests did not have enough validation BUG=630659 ========== to ========== Fixing SecurityKeySocket unit test issues The low-level security key unit tests had a few problems that I am fixing in this CL: 1.) Several unit tests didn't actually test anything 2.) The other unit tests did not have enough validation There is also some minor renaming and trimming to get more code to fit on a single line instead of spanning mulitple lines. BUG=630659 ==========
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've updated this CL to focus on improving the unit test coverage. I was initially worried about the SSH error not being sent but now that I have unit test coverage and it seems fine, I don't think I need to address that. I do think sending an SSH error every time we close the socket may be the wrong thing to do, but that will be addressed, if needed, in an upcoming CL. PTAL! https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... File remoting/host/security_key/security_key_auth_handler_posix.cc (right): https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix.cc:57: void DeleteSocket(std::unique_ptr<remoting::SecurityKeySocket> socket) {} On 2016/12/07 20:41:36, Sergey Ulanov wrote: > base::DeletePointer() can be used instead of this function Acknowledged. https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix.cc:301: base::Bind(&DeleteSocket, base::Passed(&iter->second))); On 2016/12/07 20:41:36, Sergey Ulanov wrote: > I don't think there is anything that guarantees that we will ever succeed to > send the error message, so it's possible that the the socket will never be > deleted, so I'm not sure that just releasing it here is the right thing to do. > Also it allows for other subtle bugs, e.g. can we be sure that SecurityKeySocket > will not call timeout callback later, after SecurityKeyAuthHandlerPosix is > destroyed? That's true. I had a follow-up CL which used this mechanism but I think this isn't a great approach so I will scrap it for this CL and just fix the unit tests and comments/code alignment. https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... File remoting/host/security_key/security_key_socket.cc (right): https://codereview.chromium.org/2562473002/diff/20001/remoting/host/security_... remoting/host/security_key/security_key_socket.cc:64: response_written_callback_ = response_written_callback; On 2016/12/07 20:41:36, Sergey Ulanov wrote: > If SendResponse() is called before previous response is sent then the callback > for the previous response will never be called. In this CL the callback is only > used for the error in the end, so maybe we shouldn't allow > response_written_callback only for SsendSshError(). I.e. move this line to > SendSshError() and add DCHECK(!response_written_callback_). Removed the callback mechanism. Based on having running unit tests that actually check the return values, I think the error is being sent correctly so it isn't needed here.
lgtm https://codereview.chromium.org/2562473002/diff/60001/remoting/host/security_... File remoting/host/security_key/security_key_auth_handler_posix_unittest.cc (right): https://codereview.chromium.org/2562473002/diff/60001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix_unittest.cc:48: const unsigned char kSshErrorData[] = {0x00, 0x00, 0x00, 0x01, 0x05}; Pleaes use uint8_t instead of unsigned char https://codereview.chromium.org/2562473002/diff/60001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix_unittest.cc:159: static std::string expected_data; Any reason this needs to be static? Style guide doesn't allow static vars: Variables of class type with static storage duration are forbidden: they cause hard-to-find bugs due to indeterminate order of construction and destruction. https://codereview.chromium.org/2562473002/diff/60001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix_unittest.cc:164: for (size_t i = 4; i < sizeof(kRequestData); ++i) { This loop can be expressed with one assign() call: expected_data.assign(reinterpret_cast<const char*>(kRequestData + 4), sizeof(kRequestData) - 4);
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Thanks!! https://codereview.chromium.org/2562473002/diff/60001/remoting/host/security_... File remoting/host/security_key/security_key_auth_handler_posix_unittest.cc (right): https://codereview.chromium.org/2562473002/diff/60001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix_unittest.cc:48: const unsigned char kSshErrorData[] = {0x00, 0x00, 0x00, 0x01, 0x05}; On 2016/12/14 00:17:03, Sergey Ulanov wrote: > Pleaes use uint8_t instead of unsigned char Done. https://codereview.chromium.org/2562473002/diff/60001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix_unittest.cc:159: static std::string expected_data; On 2016/12/14 00:17:03, Sergey Ulanov wrote: > Any reason this needs to be static? > Style guide doesn't allow static vars: Variables of class type with static > storage duration are forbidden: they cause hard-to-find bugs due to > indeterminate order of construction and destruction. No, I'll make it a class member. https://codereview.chromium.org/2562473002/diff/60001/remoting/host/security_... remoting/host/security_key/security_key_auth_handler_posix_unittest.cc:164: for (size_t i = 4; i < sizeof(kRequestData); ++i) { On 2016/12/14 00:17:03, Sergey Ulanov wrote: > This loop can be expressed with one assign() call: > expected_data.assign(reinterpret_cast<const char*>(kRequestData + 4), > sizeof(kRequestData) - 4); This is much cleaner, thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2562473002/#ps100001 (title: "Addressing CR Feedback #2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481684437641880,
"parent_rev": "8209d80620f8ca0683b9697f23115f82a44e8f5b", "commit_rev":
"6299b729b0950f760a4a8f0889d1e7982f783c06"}
Message was sent while issue was closed.
Description was changed from ========== Fixing SecurityKeySocket unit test issues The low-level security key unit tests had a few problems that I am fixing in this CL: 1.) Several unit tests didn't actually test anything 2.) The other unit tests did not have enough validation There is also some minor renaming and trimming to get more code to fit on a single line instead of spanning mulitple lines. BUG=630659 ========== to ========== Fixing SecurityKeySocket unit test issues The low-level security key unit tests had a few problems that I am fixing in this CL: 1.) Several unit tests didn't actually test anything 2.) The other unit tests did not have enough validation There is also some minor renaming and trimming to get more code to fit on a single line instead of spanning mulitple lines. BUG=630659 Review-Url: https://codereview.chromium.org/2562473002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fixing SecurityKeySocket unit test issues The low-level security key unit tests had a few problems that I am fixing in this CL: 1.) Several unit tests didn't actually test anything 2.) The other unit tests did not have enough validation There is also some minor renaming and trimming to get more code to fit on a single line instead of spanning mulitple lines. BUG=630659 Review-Url: https://codereview.chromium.org/2562473002 ========== to ========== Fixing SecurityKeySocket unit test issues The low-level security key unit tests had a few problems that I am fixing in this CL: 1.) Several unit tests didn't actually test anything 2.) The other unit tests did not have enough validation There is also some minor renaming and trimming to get more code to fit on a single line instead of spanning mulitple lines. BUG=630659 Committed: https://crrev.com/865c534bef93f875acf0073efcbbf79e201339fd Cr-Commit-Position: refs/heads/master@{#438408} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/865c534bef93f875acf0073efcbbf79e201339fd Cr-Commit-Position: refs/heads/master@{#438408} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
