Skip to content
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

ssh: ignore io.EOF from sftp.Server.Serve #8670

Merged
merged 1 commit into from
Jul 21, 2023
Merged

ssh: ignore io.EOF from sftp.Server.Serve #8670

merged 1 commit into from
Jul 21, 2023

Conversation

dsnet
Copy link
Member

@dsnet dsnet commented Jul 21, 2023

If the connection provided to sftp.NewServer is closed, Serve returns the io.EOF error verbatim from io.Reader.Read. This is an odd error since this is an expected situation, so we manually ignore io.EOF.
This is somewhat buggy since the sftp package itself incorrectly reports io.EOF in cases where it should actually be reporting io.ErrUnexpectedEOF.
See pkg/sftp#554 which patches Serve to return nil on clean closes and fixes buggy uses of io.ReadFull.

Fixes #8592

If the connection provided to sftp.NewServer is closed,
Serve returns the io.EOF error verbatim from io.Reader.Read.
This is an odd error since this is an expected situation,
so we manually ignore io.EOF.
This is somewhat buggy since the sftp package itself
incorrectly reports io.EOF in cases where it should actually
be reporting io.ErrUnexpectedEOF.
See pkg/sftp#554 which patches Serve to
return nil on clean closes and fixes buggy uses of io.ReadFull.

Fixes #8592

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
@dsnet dsnet requested review from bradfitz and maisem July 21, 2023 08:53
@dsnet
Copy link
Member Author

dsnet commented Jul 21, 2023

I tested this manually and then started to read the "sftp" package code to understand why it returns io.EOF, then I discovered other bugs in that package (see pkg/sftp#554).

I couldn't figure out an easy way to write a test. The entire beIncubator function is untested.

Copy link
Member

@bradfitz bradfitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bunch of stuff in that sftp package I want to fix. Maybe it's time we fork it?

@bradfitz
Copy link
Member

Should we cherry-pick this back to the 1.46 branch? Probably?

cc @DentonGentry

@DentonGentry
Copy link
Contributor

Putting it in 1.46.1 seems reasonable.

@bradfitz
Copy link
Member

Then I'll merge this & include it in my cherry-pick PR I'm about to send.

@bradfitz bradfitz merged commit bb4b35e into main Jul 21, 2023
36 checks passed
@bradfitz bradfitz deleted the dsnet/ssh-sftp branch July 21, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scp giving EOF and exit status 1
3 participants