Skip the blank line some servers end an MLSD listing with - #77
Open
christhomas wants to merge 1 commit into
Open
Skip the blank line some servers end an MLSD listing with#77christhomas wants to merge 1 commit into
christhomas wants to merge 1 commit into
Conversation
Fixes secsy#45. A trailing empty line is not an entry, and parsing it as one failed the whole listing — so a directory that was perfectly readable came back as "failed parsing MLST entry:" with nothing after the colon. parseLIST already skips its own non-entry line ("total 404456"), so the two parsers now agree that a line which is not an entry is not an error either. Whitespace-only lines count, since a server sending "\\r" is sending the same nothing. ## A second-order effect, which the fix would otherwise have introduced Stat shares the parser and expects the one line it passes in to be the answer. Reporting "not an entry" as (nil, nil) is right for a listing and wrong there: Stat would have returned a nil FileInfo with a nil error, and a caller that checked the error would then dereference nothing. Stat now rejects an MLST reply whose entry line is blank. That was confirmed red — both test servers returned a nil FileInfo and no error before the guard. The malformed case is still an error, so skipping blank lines has not become skipping anything inconvenient; there is a test for that.
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #45.
A trailing empty line is not an entry, and parsing it as one fails the whole listing — so a directory that is perfectly readable comes back as:
with nothing after the colon, which is what makes it hard to place.
parseLISTalready skips its own non-entry line (total 404456), so the two parsers now agree that a line which is not an entry is not an error either. Whitespace-only lines count, since a server sending\ris sending the same nothing.A second-order effect worth pointing out
Statshares this parser and expects the single line it passes in to be the answer. Reporting "not an entry" as(nil, nil)is right for a listing and wrong there:Statwould return a nilos.FileInfowith a nil error, and a caller that checked the error would then dereference nothing.So
Statnow rejects an MLST reply whose entry line is blank. I confirmed that red first — both test servers returned a nilFileInfoand no error before the guard went in.The malformed case is still an error, so skipping blank lines has not become skipping anything inconvenient. There is a test for that.
@olegabr's suggestion in the issue was exactly this; @tommed also has a fix in a fork.
One note on running the tests: this branch is off
master, soTestMainstill requires./build_test_server.sh— the tests here need no server, but they cannot start without one. I verified them via the container harness in #73.