You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by GitBox <gi...@apache.org> on 2022/03/31 13:51:47 UTC
[GitHub] [mina-sshd] Vlatombe opened a new pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Vlatombe opened a new pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214
[SSHD-1257](https://issues.apache.org/jira/browse/SSHD-1257)
If the command implementation closes the output stream at the end of its execution, the ssh server hangs.
This changes the `ChannelOutputStream#flush` to become a no-op if it has been closed already.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] Vlatombe edited a comment on pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
Vlatombe edited a comment on pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#issuecomment-1084944783
Just thought of an easier alternative... `close` takes care of flushing already and is a no-op if already closed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#discussion_r839816593
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/channel/ChannelOutputStream.java
##########
@@ -195,9 +195,7 @@ public synchronized void write(byte[] buf, int s, int l) throws IOException {
public synchronized void flush() throws IOException {
Channel channel = getChannel();
if (!isOpen()) {
Review comment:
I agree with [~tomaswolf]-s suggestion...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#discussion_r840270775
##########
File path: sshd-core/src/main/java/org/apache/sshd/server/channel/ChannelSession.java
##########
@@ -908,7 +908,7 @@ protected void closeShell(int exitValue, boolean closeImmediately) throws IOExce
if (!isClosing()) {
if (out != null) {
- out.flush();
Review comment:
> out.close() works because this is indeed the last use of this stream. Looks fine to me...
I trust your judgment @tomaswolf
> The intent is obviously to get all data from the command sent before the EOF is sent. But why not also flush or close err? What happens if the command writes something to the error stream? err gets closed only in the close() call in line 916, so that would flush it after the EOF (and the exit status) was sent. That's probably not OK.
I tend to agree with you...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] Vlatombe commented on pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
Vlatombe commented on pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#issuecomment-1084944783
Just thought of an easier alternative...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#issuecomment-1084756936
But wouldn't it be more appropriate then to do in`ChannelSession` line 910
```
if (out != null && out.isOpen()) {
out.flush();
}
```
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#discussion_r839815525
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/channel/ChannelOutputStream.java
##########
@@ -195,9 +195,7 @@ public synchronized void write(byte[] buf, int s, int l) throws IOException {
public synchronized void flush() throws IOException {
Channel channel = getChannel();
if (!isOpen()) {
Review comment:
I don't think this is a good idea - what if the user meant to flush the stream but did not notice it is closed. The code would simply "swallow" the exception. AFAIK the stream contract is clear: do not call **any** method after calling *close()* (one *can* call *close()* as many times as one wants since it is idempotent),
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] Vlatombe commented on pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
Vlatombe commented on pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#issuecomment-1084658513
@jglick https://github.com/apache/mina-sshd/blob/da2958172281ef20e51681afe036bd42c9cf843a/sshd-core/src/main/java/org/apache/sshd/server/channel/ChannelSession.java#L910-L912
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] Vlatombe commented on pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
Vlatombe commented on pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#issuecomment-1084934389
> But wouldn't it be more appropriate then to do inChannelSession line 910
It's not as simple, because `isOpen` is only defined on `ChannelOutputStream` but depending on cases it can be wrapped in `LoggingFilterOutputStream`.
See https://github.com/apache/mina-sshd/blob/da2958172281ef20e51681afe036bd42c9cf843a/sshd-core/src/main/java/org/apache/sshd/server/channel/ChannelSession.java#L733-L743
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#discussion_r840186180
##########
File path: sshd-core/src/main/java/org/apache/sshd/server/channel/ChannelSession.java
##########
@@ -908,7 +908,7 @@ protected void closeShell(int exitValue, boolean closeImmediately) throws IOExce
if (!isClosing()) {
if (out != null) {
- out.flush();
Review comment:
Not sure about this... "smells fishy" as well , though I cannot put my fiinger on it - will have to think about it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #214: [SSHD-1257] Make ChannelOutputStream#flush no-op if the stream is already closed
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on a change in pull request #214:
URL: https://github.com/apache/mina-sshd/pull/214#discussion_r840247098
##########
File path: sshd-core/src/main/java/org/apache/sshd/server/channel/ChannelSession.java
##########
@@ -908,7 +908,7 @@ protected void closeShell(int exitValue, boolean closeImmediately) throws IOExce
if (!isClosing()) {
if (out != null) {
- out.flush();
Review comment:
`out.close()` works because this is indeed the last use of this stream. Looks fine to me...
Unrelated to this change:
But the code is a bit strange anyway. The intent is obviously to get all data from the command sent before the EOF is sent. But why not also flush or close `err`? What happens if the command writes something to the error stream? `err` gets closed only in the `close()` call in line 916, so that would flush it after the EOF (and the exit status) was sent. That's probably not OK.
And anyway: stderr is traditionally "unbuffered", i.e., flushed on newlines. I don't see anything in ChannelOutputStream that would implement that for SSH_MSG_CHANNEL_EXTENDED_DATA. Maybe that's OK; the command would be responsible for flushing it frequently then if desired.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org