Web UI: Explicitly strip newlines from chunks when uploading

[rdmark]

No description provided.

[uweseimet]

@rdmark Just a question on this. Please bear with me if this question does not apply for this ticket, because I have not digged deeper into this.
In general, issues caused by newlines are typical for cases where the upload protocol/format is not a binary format. A good example is ftp, where you can choose between text/ASCII and binary transfer format. If you transfer binaries with text format enabled you run into very similar issues with ftp like the one this ticket is addressing. Can it be that there is a general problem with uploading binaries in the web UI, e.g. because the code or the libraries used are trying to deal with text instead of binary format? Where do the newlines at the end of a chunk come from?

One more thing: When you strip a newline, how do you know that the newline (the binary representation of it) does not belong there? Depending on how you define a newline (depending on the platform it can be CR, LF or CR/LF), there can be chunks that happen to have these characters at their end. Stripping them in this case would corrupt the uploaded data.

[nucleogenic]

Looking at what's been discussed in the ticket, the change seems reasonable enough. It would be better if we could understand why the issue isn't reproducible between users though. There's seemingly a component to this problem we don't understand 🤔

[uweseimet]

@nucleogenic I would have appreciated if somebody had explained why my concerns are not justified and a sequence of line feed characters that is actually a valid part of the binary cannot just be stripped. As long as one does not fully understand a problem one cannot resolve it.

[rdmark]

@uweseimet Yes, It's a mystery where the CRLF comes from. Your theory sounds believable, that somewhere in the software stack the data is treated as text and not binary.

In the python code we create the file object in binary append mode, as per the documentation: https://docs.python.org/3/tutorial/inputoutput.html#reading-and-writing-files

The one thing I can imagine is that a corner case bug with dropzone.js and the particular version of Chrome that the reporter is using.

I'm hoping that we can get some more feedback and test data from the user and then decide what to do next.

[uweseimet]

@rdmark In order to reproduce this problem, what about creating an image file that only contains sequences of CR, CR/LF and LF and that is big enough to require several chunks? For testing the upload there is no need for an image with valid filesystems.
If there is an issue with binary/ASCII format, it might also depend on the charsets/locales used on the client and the server. If this reproduce the problem it helps resolving it. If there is no problem with this approach (and without changing anything in piscsi) there may not be a problem at all, or it is, just like you say, something related to issues with a user's setup. At least as long as only a single user has this problem.

Edit: In case you run such a test, it's best to use three images. Only only with CR characters, the second only with LF, and the third only with CR/LF. This way you will definitely have chunks of the type that might be causing issues.
Creating such a file should work with dd, e.g. similar to

dd if=/dev/zero bs=1024 count=1 | tr '\000' '1' > test.img

With bs the size of the file to create, and the second argument of tr the character the file shall consist of.
Such a file also shows the problem I tried to point out: How do you know which of the characters to strip and which not? Just saying "the ones at the end of each chunk" is IMHO not correct. Any resolution for this ticket would have to ensure that such files are uploaded without being modified.

[rdmark]

@uweseimet A file consisting of nothing but CRLF does indeed get corrupted with the code in this PR. (And works before the change.)

One idea I had, is to add a check for the size of a transferred chunk, and strip trailing line endings only when the actual size exceeds the expected size. Thoughts?

[uweseimet]

IMO this is not a solution. I bet that you are missing something.Am 29.10.2023 09:16 schrieb Daniel Markstedt ***@***.***>: @uweseimet A file consisting of nothing but CRLF does indeed get corrupted with the code in this PR. (And works before the change.) One idea I had, is to add a check for the size of a transferred chunk, and strip trailing line endings only when the actual size exceeds the expected size. Thoughts? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[rdmark]

I added some debug output and will ask the original reporter to test once they respond. Logging should at least tell us if the chunk size and offsets are correct.

[uweseimet]

I recommend not to change anything until it is fully clear where this problem comes from, and there should be more than one user who can reproduce it.All in all either the technologies you are using cannot deal with binary uploads (unlikely), you are not using them correctly and are missing something, or there is no problem and the respective user's setup is some broken.IMO there are no other alternatives.Am 29.10.2023 12:33 schrieb Daniel Markstedt ***@***.***>: I added some debug output and will ask the original reporter to test once they respond. Logging should at least tell us if the chunk size and offsets are correct. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
5.7% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[uweseimet]

@rdmark Correct me if I am wrong: The latest comments in #1157 say that there is no problem at all, don't they?

[rdmark]

I want to ask the original reporter to try one more thing: the current develop code... just to make sure that the adding of the logging didn't have an effect on their corner case.

Edit: never mind, I see now that they already tested that!