Bump to latest grpc version#5299
Conversation
This is ostensibly for CVE-2023-44487. Note: the netty-tcnative-boringssl-static version does not need to be updated for this.
devinrsmith
added
grpc
security
NoDocumentationNeeded
version-bump
ReleaseNotesNeeded
|
Note: I did some manual editting of io.grpc.* impls, along with what seemed like correct behavior. I'm not sure exactly the delta we want / need between our versions and grpc proper versions. Colin, do we still needs to maintain these layers? |
|
Yes, by the time grpc-java landed our changes, our impl had drifted, and we haven't taken the time to propose api changes back to them again. I'll go over the remaining changes in our |
niloc132
left a comment
niloc132
left a comment
There was a problem hiding this comment.
I think we should revert some of these code changes around flush, see comments in the review.
I'll propose one commit for that (after any discussion), and then also port other changes from upstream - looks like we could use a logging change (minor perf improvement), a dependency cleanup, and a possible data race.
| if (flush && websocketSession.getBasicRemote().getBatchingAllowed()) { | ||
| websocketSession.getBasicRemote().flushBatch(); | ||
| } |
There was a problem hiding this comment.
Technically correct, but will never execute - we never setBatchingAllowed(true), so this will never run.
| try { | ||
| // send in two separate payloads | ||
| websocketSession.getBasicRemote().sendBinary(prefix); | ||
| websocketSession.getBasicRemote().sendBinary(prefix, false); |
There was a problem hiding this comment.
We don't need to do this - this means that the client can't begin to parse the message until the whole thing is sent. Technically it sort of does this anyway, but the client is implemented to buffer the partial messages it received.
This is unrelated to flushing anyway, and will not prevent flushing. Instead a websocket Binary Data frame will be sent after calling this with fin=false, rather than deferring sending at all (also called a "fragmented message").
| } catch (IllegalStateException | IOException e) { | ||
| logger.log(WARNING, String.format("[{%s}] Exception when flushBuffer", logId), e); | ||
| cancel(Status.fromThrowable(e)); | ||
| if (flush) { |
There was a problem hiding this comment.
Reviewing grpc/grpc-java@8c05455 which introduced this API change, this is the opposite of what we need (not that we have the direct capability to control it here, at a glance): the bug being fixed is that the default is presumed to be to flush headers when this method was called, and the change is to say "actually, for unary (or client-streaming), suppress flushing when writing headers, wait until the first/only payload".
Upstream does not add any extra flush code here, and we shouldn't either - the goal isn't to say "ensure you flush right now when flush=true", but "if flush=false, ensure you don't flush".
|
Advice from @devinrsmith, while updating other versions:
|
|
Superseded by #6301. |
3 participants
This is ostensibly for CVE-2023-44487.
Note: the netty-tcnative-boringssl-static version does not need to be updated for this.