Spurious error decoding multi-byte output from ShellSession

[christian-intra2net]

We are using avocado to test our email filtering pipeline. We find lots of test files by running a "find" command on the VM through a RemoteSession and then perfom tests with each of these. One of the test files' name contains lots of traditional Chinese characters and sometimes there were errors only with that file. We spent quite some time to notice that the filename is not always transferred correctly from the VM to the calling host: sometimes one of the file's characters is just missing.

Further investigation revealed the problem in the ShellSession's decoding: it reads up to 1024 bytes from the VM, decodes those, and appends the decoded characters to the complete output, ignoring errors. If the output size is larger than 1024 bytes, and the 1023rd byte happens to be the first or second of the Chinese character bytes (each character requires 3 bytes in our utf8-environment), then decoding fails silently and that one character is missing from the transferred text.

The same erroneous decoding happens in Tail._tail. I have created a reproducer, which hopefully clarifies the problem:

encoding_test.py

[christian-intra2net]

I am working on a suggestion for a solution, standby for PR...

[christian-intra2net]

One thing that struck me during fixing: there is a lot of "ignore"-type error handling in bytes.decode(...). This kind of hides problems like the one I fixed here, an error handling of "replace" would have made discovery of this problem much easier. Error-handling policy "replace" will replace problematic byte sequences with the official unicode replacement char '\uFFFD', which is a great way to forward the information "there has been a decoding problem" to the caller code and ultimately the user.
Is there a good reason why the error handling policy here is "ignore"?

[ldoktor]

One thing that struck me during fixing: there is a lot of "ignore"-type error handling in bytes.decode(...). This kind of hides problems like the one I fixed here, an error handling of "replace" would have made discovery of this problem much easier. Error-handling policy "replace" will replace problematic byte sequences with the official unicode replacement char '\uFFFD', which is a great way to forward the information "there has been a decoding problem" to the caller code and ultimately the user. Is there a good reason why the error handling policy here is "ignore"?

Hello @christian-intra2net IIRC back then replace was not supported in really old pythons, which is why we used ignore. These days it should be safe to switch to replace although some tests might break when extra characters are injected eg. by serial console. @pevogam what do you think? (better explicit than hidden...)

[pevogam]

One thing that struck me during fixing: there is a lot of "ignore"-type error handling in bytes.decode(...). This kind of hides problems like the one I fixed here, an error handling of "replace" would have made discovery of this problem much easier. Error-handling policy "replace" will replace problematic byte sequences with the official unicode replacement char '\uFFFD', which is a great way to forward the information "there has been a decoding problem" to the caller code and ultimately the user. Is there a good reason why the error handling policy here is "ignore"?

Hello @christian-intra2net IIRC back then replace was not supported in really old pythons, which is why we used ignore. These days it should be safe to switch to replace although some tests might break when extra characters are injected eg. by serial console. @pevogam what do you think? (better explicit than hidden...)

I support the explicitness improvement. @ldoktor Do you think any of this may affect downstream users in any negative way? Adjusting the expectations of a few failing tests to contract a new and better behavior should be fine, I just worry for production code side effects.

[ldoktor]

One thing that struck me during fixing: there is a lot of "ignore"-type error handling in bytes.decode(...). This kind of hides problems like the one I fixed here, an error handling of "replace" would have made discovery of this problem much easier. Error-handling policy "replace" will replace problematic byte sequences with the official unicode replacement char '\uFFFD', which is a great way to forward the information "there has been a decoding problem" to the caller code and ultimately the user. Is there a good reason why the error handling policy here is "ignore"?

Hello @christian-intra2net IIRC back then replace was not supported in really old pythons, which is why we used ignore. These days it should be safe to switch to replace although some tests might break when extra characters are injected eg. by serial console. @pevogam what do you think? (better explicit than hidden...)

I support the explicitness improvement. @ldoktor Do you think any of this may affect downstream users in any negative way? Adjusting the expectations of a few failing tests to contract a new and better behavior should be fine, I just worry for production code side effects.

TBH I don't know how often this is. I remember several serial console tests which relied on utils.astring.strip_console_codes to basically strip everything that didn't belong there and \xff would persist and might break explicit checks. At the same time it's not a console code so stripping it there doesn't seem right.

I'm more inclined to explicit so I'd prefer to switch to replace, mention it in release notes and withdraw if too many people complains. @smitterl @harvey0100 @clebergnu any takes on this?