-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add doPrivilege blocks for socket connect ops in repository-hdfs #22793
Conversation
In hdfs we are calling |
We do this to limit what craziness hdfs can do (and know exactly what it requires). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment looks great so far
try { | ||
SpecialPermission.check(); | ||
// FSDataInputStream can open connection on read() | ||
return AccessController.doPrivileged((PrivilegedExceptionAction<Integer>) is::read); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we maybe try to trigger this differently? I mean can we for instance try to call #available()
or can we maybe read the first byte on open and wrap in a BufferedInputStream
and then do this:
InputStream stream = is.isMarkSupported() ? is : new BufferedInputStream(is);
// do the following in doPrivileged?
stream.mark(1);
stream.skip(1);
stream.reset();
return stream;
@s1monw In my testing available() was not doing the trick. So I went with the buffered, mark, skip, reset approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left one more comment we are close I think
} | ||
} | ||
}; | ||
InputStream stream = is.markSupported() ? is : new BufferedInputStream(is); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we run into an exception here we have to close the stream.
we usually do this:
boolean success = false;
try {
// do something with the stream
success = true;
return stream;
} finally {
if (success == false) {
IOUtils.closeWhileHandlingException(stream);
}
}
Note to other reviews: Simon and I talked about this for a while and decided it was safest to return a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…-hdfs (elastic#22793)" Only pulled the relevant changes - such as the Priveleged input stream implementation for HDFS.
This is related to #22116. The repository-hdfs plugin opens socket
connections. As
SocketPermission
is transitioned out of core, hdfswill require
connect
permission. This pull request wraps operationsthat require this permission in
doPrivileged
blocks.