You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2022/05/10 02:36:21 UTC
[kudu-CR] [client] prohibit copying/assigning of ResourceMetrics
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18510
Change subject: [client] prohibit copying/assigning of ResourceMetrics
......................................................................
[client] prohibit copying/assigning of ResourceMetrics
Since ResourceMetrics contains a raw pointer, it makes it dangerous
since its auto-generated copy constructor and assignment operator
are not private. Somebody could write something like
{
KuduScanner scanner();
{
ResouceMetrics m = session->GetResourceMetrics();
...
}
// Continue scanning activity with the 'scanner'.
...
}
or
{
KuduSession session(...);
{
ResouceMetrics m = session->GetWriteOpMetrics();
...
}
// Continue writing activity with the 'session'.
...
}
and hit a silent memory corruption issue since that's the use-after-free
condition for the KuduScanner and KuduSession instances when the data
behind the raw pointer in ResourceMetrics::data_ is accessed later on.
This patch breaks the ABI compatibility for the kudu_client C++ library,
but this would surface only if there is a dangerous code like above
in the Kudu application linked with the library. It's much worse having
such a hidden memory corruption issue in an application than hitting
a build breakage due to a linkage error which is quite simple to fix.
This is a follow-up to ece7b5653998db318e4baa5d57f27ba3a836731d.
Change-Id: I602cc4e194a975752687d13d525e44043955a5cf
---
M src/kudu/client/resource_metrics.h
1 file changed, 6 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/18510/1
--
To view, visit http://gerrit.cloudera.org:8080/18510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I602cc4e194a975752687d13d525e44043955a5cf
Gerrit-Change-Number: 18510
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
[kudu-CR] [client] prohibit copying/assigning of ResourceMetrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18510 )
Change subject: [client] prohibit copying/assigning of ResourceMetrics
......................................................................
[client] prohibit copying/assigning of ResourceMetrics
Since ResourceMetrics contains a raw pointer, it makes it dangerous
since its auto-generated copy constructor and assignment operator
are not private. Somebody could write something like
{
KuduScanner scanner();
{
ResouceMetrics m = session->GetResourceMetrics();
...
}
// Continue scanning activity with the 'scanner'.
...
}
or
{
KuduSession session(...);
{
ResouceMetrics m = session->GetWriteOpMetrics();
...
}
// Continue writing activity with the 'session'.
...
}
and hit a silent memory corruption issue since that's the use-after-free
condition for the KuduScanner and KuduSession instances when the data
behind the raw pointer in ResourceMetrics::data_ is accessed later on.
This patch breaks the ABI compatibility for the kudu_client C++ library,
but this would surface only if there is a dangerous code like above
in the Kudu application linked with the library. It's much worse having
such a hidden memory corruption issue in an application than hitting
a build breakage due to a linkage error which is quite simple to fix.
This is a follow-up to ece7b5653998db318e4baa5d57f27ba3a836731d.
Change-Id: I602cc4e194a975752687d13d525e44043955a5cf
Reviewed-on: http://gerrit.cloudera.org:8080/18510
Tested-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Riza Suminto <ri...@cloudera.com>
Reviewed-by: Attila Bukor <ab...@apache.org>
---
M src/kudu/client/resource_metrics.h
1 file changed, 6 insertions(+), 2 deletions(-)
Approvals:
Alexey Serbin: Verified
Riza Suminto: Looks good to me, but someone else must approve
Attila Bukor: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I602cc4e194a975752687d13d525e44043955a5cf
Gerrit-Change-Number: 18510
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [client] prohibit copying/assigning of ResourceMetrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [client] prohibit copying/assigning of ResourceMetrics
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/18510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I602cc4e194a975752687d13d525e44043955a5cf
Gerrit-Change-Number: 18510
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [client] prohibit copying/assigning of ResourceMetrics
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18510 )
Change subject: [client] prohibit copying/assigning of ResourceMetrics
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I602cc4e194a975752687d13d525e44043955a5cf
Gerrit-Change-Number: 18510
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 May 2022 17:34:09 +0000
Gerrit-HasComments: No
[kudu-CR] [client] prohibit copying/assigning of ResourceMetrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18510 )
Change subject: [client] prohibit copying/assigning of ResourceMetrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18510/1//COMMIT_MSG
Commit Message:
PS1:
We could use the special '= delete' syntax for the copy constructor and the assignment operator (that's available since C++11), but we are tied to be C++98 compatible in the Kudu C++ client API, unfortunately.
--
To view, visit http://gerrit.cloudera.org:8080/18510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I602cc4e194a975752687d13d525e44043955a5cf
Gerrit-Change-Number: 18510
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 May 2022 15:06:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] prohibit copying/assigning of ResourceMetrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18510 )
Change subject: [client] prohibit copying/assigning of ResourceMetrics
......................................................................
Patch Set 1: Verified+1
unrelated failure in DisableWriteWhenExceedingQuotaTest.TestDisableWritePrivilegeWhenExceedingSizeQuota
--
To view, visit http://gerrit.cloudera.org:8080/18510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I602cc4e194a975752687d13d525e44043955a5cf
Gerrit-Change-Number: 18510
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 May 2022 03:41:48 +0000
Gerrit-HasComments: No
[kudu-CR] [client] prohibit copying/assigning of ResourceMetrics
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18510 )
Change subject: [client] prohibit copying/assigning of ResourceMetrics
......................................................................
Patch Set 1: Code-Review+1
Thank you for fixing this issue! The code looks good to me.
--
To view, visit http://gerrit.cloudera.org:8080/18510
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I602cc4e194a975752687d13d525e44043955a5cf
Gerrit-Change-Number: 18510
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 10 May 2022 05:13:37 +0000
Gerrit-HasComments: No