You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Edward Fancher (Code Review)" <ge...@cloudera.org> on 2017/06/09 20:39:19 UTC
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Edward Fancher has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7141
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/1
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
[kudu-CR] KUDU-2004: Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7141
to look at the new patch set (#10).
Change subject: KUDU-2004: Undefined behavior in TlsSocket::Writev()
......................................................................
KUDU-2004: Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.h
2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/10
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2004: Undefined behavior in TlsSocket::Writev()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-2004: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7141/9/src/kudu/util/net/socket.h
File src/kudu/util/net/socket.h:
PS9, Line 123: // If the result is an error then the value in nwritten is not valid.
: // Else if nwritten is zero, but there was not an error, you should try again.
Did you see my suggestion in CR 4 for this comment?
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Edward Fancher has posted comments on this change.
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7141/5/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
Line 80: if (!write_status.ok()) break;
> To be honest I don't think this comment is necessary either; the "Status s
Done
Line 81:
> This comment duplicates the one on L78.
Done
http://gerrit.cloudera.org:8080/#/c/7141/5/src/kudu/util/net/socket.h
File src/kudu/util/net/socket.h:
Line 124: // If amt == nwritten == 0, this was a no-op.
> Don't you mean "this was a no-op"? That is, if nwritten == 0 after the call
is => was.
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 4:
(3 comments)
The fix looks good. +1 to what Dan said
http://gerrit.cloudera.org:8080/#/c/7141/4/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
PS4, Line 80: // Don't want to update the number written if there was an error.
: // We don't return yet, since we want to turn off the Tcp Cork.
I think this comment can be simplified by saying something like:
// If Write() returns an error then the value of 'nwritten' is undefined.
PS4, Line 84: // nwritten should have the correct amount written.
How about:
// If we were unable to write the requested number of bytes we return early, but not before uncorking.
Or something similar.
http://gerrit.cloudera.org:8080/#/c/7141/4/src/kudu/security/tls_socket.h
File src/kudu/security/tls_socket.h:
Line 35: // If the result is an error then the value in nwritten is not valid.
> Could you instead add these docs to the parent class in socket.h, they shou
+1 for moving to class declaration for Socket.
For things like this that are wrapping POSIX calls, I usually get inspiration from the man pages i.e. http://man7.org/linux/man-pages/man2/write.2.html
So I would just write something like
// Write up to 'amt' bytes from 'buf' to the socket. The number of bytes
// actually written will be stored in 'nwritten'. If an error is returned,
// the value of 'nwritten' is undefined.
I don't think you need to detail what happens when amt == 0 since it seems intuitive to me.
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2004. Undefined behavior in TlsSocket::Writev()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-2004. Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7141
to look at the new patch set (#7).
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.h
2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/7
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7141
to look at the new patch set (#5).
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.h
2 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/5
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7141/4/src/kudu/security/tls_socket.h
File src/kudu/security/tls_socket.h:
Line 35: // If the result is an error then the value in nwritten is not valid.
Could you instead add these docs to the parent class in socket.h, they should apply equally to all implementations.
Line 44: // iov[nwritten] to iov[iov_len-1] need to be written again.
The nwritten index isn't valid on the iov array. I'd just simplify and say '..., but not all the bytes were written, the unwritten bytes must be retried. See writev(2) for more information.'.
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2004. Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7141
to look at the new patch set (#11).
Change subject: KUDU-2004. Undefined behavior in TlsSocket::Writev()
......................................................................
KUDU-2004. Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.h
2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/11
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7141
to look at the new patch set (#8).
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.h
2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/8
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7141
to look at the new patch set (#6).
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.h
2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/6
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7141
to look at the new patch set (#3).
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/3
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2004: Undefined behavior in TlsSocket::Writev()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-2004: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7141
to look at the new patch set (#2).
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/security/tls_socket.h
2 files changed, 13 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/2
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2004. Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Edward Fancher has posted comments on this change.
Change subject: KUDU-2004. Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7141/8//COMMIT_MSG
Commit Message:
Line 7: KUDU-2004. Undefined behavior in TlsSocket::Writev()
> err only one space after the period.
Done
Line 7: KUDU-2004. Undefined behavior in TlsSocket::Writev()
> When there's a corresponding jira we prefix the first line like this:
Done
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7141/8//COMMIT_MSG
Commit Message:
Line 7: Undefined behavior in TlsSocket::Writev()
When there's a corresponding jira we prefix the first line like this:
KUDU-2004. Undefined behavior in TlsSocket::Writev()
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7141/5/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
Line 81: // We don't return yet, since we want to turn off the Tcp Cork.
> This comment duplicates the one on L78.
Ah good point
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2004: Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7141
to look at the new patch set (#9).
Change subject: KUDU-2004: Undefined behavior in TlsSocket::Writev()
......................................................................
KUDU-2004: Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
---
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.h
2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7141/9
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Edward Fancher has posted comments on this change.
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7141/4/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
PS4, Line 80: if (!write_status.ok()) break;
:
> I think this comment can be simplified by saying something like:
Removed this per Adar.
PS4, Line 84: if (*nwritten < frame_size) break;
> How about:
Removed this per Adar.
http://gerrit.cloudera.org:8080/#/c/7141/4/src/kudu/security/tls_socket.h
File src/kudu/security/tls_socket.h:
Line 35: Status Write(const uint8_t *buf, int32_t amt, int32_t *nwritten) override WARN_UNUSED_RESULT;
> +1 for moving to class declaration for Socket.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2004. Undefined behavior in TlsSocket::Writev()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: KUDU-2004. Undefined behavior in TlsSocket::Writev()
......................................................................
KUDU-2004. Undefined behavior in TlsSocket::Writev()
TlsSocket::Writev() was attempting to use the value of nwritten from
TlsSocket::Write(), but in the case of an error that value was never
set or initialized. A simple check to make sure the result from
TlsSocket::Write() wasn't an error was added, otherwise we break out of
the write loop to cleanup and return the error (thus skipping the line
that uses nwritten)
Dist job result from before the fix:
http://dist-test.cloudera.org/job?job_id=efan.1496860112.16151
Dist job result from after the fix:
http://dist-test.cloudera.org/job?job_id=efan.1497036430.19311
Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Reviewed-on: http://gerrit.cloudera.org:8080/7141
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/security/tls_socket.cc
M src/kudu/util/net/socket.h
2 files changed, 10 insertions(+), 0 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-2004: Undefined behavior in TlsSocket::Writev()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-2004: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7141/10//COMMIT_MSG
Commit Message:
PS10, Line 7: :
nit: Mind using a period instead of a colon here per JD's comment?
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2004: Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Edward Fancher has posted comments on this change.
Change subject: KUDU-2004: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7141/9/src/kudu/util/net/socket.h
File src/kudu/util/net/socket.h:
PS9, Line 123: // Write up to 'amt' bytes from 'buf' to the socket. The number of bytes
: // actually written will be stored in 'nwritten'. If an error is returned,
> Did you see my suggestion in CR 4 for this comment?
Sorry, I made a mistake in translating the line numbers from tls_socket.h to socket.h and I fixed the wrong comment. Should be better now.
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2004. Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Edward Fancher has posted comments on this change.
Change subject: KUDU-2004. Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7141/10//COMMIT_MSG
Commit Message:
PS10, Line 7: .
> nit: Mind using a period instead of a colon here per JD's comment?
Done
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 5:
(3 comments)
Fix looks fine, just have some suggestions around commenting.
http://gerrit.cloudera.org:8080/#/c/7141/5/src/kudu/security/tls_socket.cc
File src/kudu/security/tls_socket.cc:
Line 80: // Don't want to update the number written if there was an error.
To be honest I don't think this comment is necessary either; the "Status s = Foo(); if (!s.ok()) break;" idiom is common enough that it doesn't need explaining.
Line 81: // We don't return yet, since we want to turn off the Tcp Cork.
This comment duplicates the one on L78.
http://gerrit.cloudera.org:8080/#/c/7141/5/src/kudu/util/net/socket.h
File src/kudu/util/net/socket.h:
Line 124: // If amt == nwritten == 0, this is a no-op.
Don't you mean "this was a no-op"? That is, if nwritten == 0 after the call? Why would it matter what nwritten was before the call?
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7141/8//COMMIT_MSG
Commit Message:
Line 7: Undefined behavior in TlsSocket::Writev()
> When there's a corresponding jira we prefix the first line like this:
err only one space after the period.
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Undefined behavior in TlsSocket::Writev()
Posted by "Edward Fancher (Code Review)" <ge...@cloudera.org>.
Edward Fancher has posted comments on this change.
Change subject: Undefined behavior in TlsSocket::Writev()
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7141/4/src/kudu/security/tls_socket.h
File src/kudu/security/tls_socket.h:
Line 35: Status Write(const uint8_t *buf, int32_t amt, int32_t *nwritten) override WARN_UNUSED_RESULT;
> Could you instead add these docs to the parent class in socket.h, they shou
Done
Line 44:
> The nwritten index isn't valid on the iov array. I'd just simplify and say
Done
--
To view, visit http://gerrit.cloudera.org:8080/7141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Edward Fancher <ef...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: Yes