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

UnicodeDecodeError: 'utf-8' codec can't decode byte 0xbd in position 6: invalid start byte #20791

Closed
jciesla opened this issue Jul 22, 2024 · 11 comments · Fixed by #20840
Closed
Assignees
Labels

Comments

@jciesla
Copy link

jciesla commented Jul 22, 2024

Explain what happens

In the latest versions of Cockpit I sometimes have difficulty opening files in the terminal tool with vim. The problem is intermittent but occurs often. Sometimes I can open the file, sometimes I can't (just a black screen), and sometimes I can open the file but the colors are changed.

Cockpit v321
Python 3.12.4
Kernel 6.6.35
Systemd 255

  1. Open a cockpit session.
  2. Open the terminal tool.
  3. Execute vim to edit a text file.

By exemple, for editing the file /etc/e2scrub.conf (can be any other file) :

Edition is working as expected:
image

Edition is working but with other colors:
image

Edition is not working:
image

When result is not the expected (wrong color or not viewable), I get python errors on journald (please see the attached traces).

Version of Cockpit

~321

Where is the problem in Cockpit?

Terminal

Server operating system

other

Server operating system version

No response

What browsers are you using?

Chrome

System log

Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]: asyncio-ERROR: Exception in callback _Transport._read_ready()
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]: handle: <Handle _Transport._read_ready()>
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]: Traceback (most recent call last):
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:   File "/usr/lib64/python3.12/asyncio/events.py", line 88, in _run
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:     self._context.run(self._callback, *self._args)
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:   File "/usr/lib/python3.12/site-packages/cockpit/transports.py", line 110, in _read_ready
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:     self._protocol.data_received(data)
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:   File "/usr/lib/python3.12/site-packages/cockpit/channel.py", line 427, in data_received
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:     if not self.send_data(data):
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:            ^^^^^^^^^^^^^^^^^^^^
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:   File "/usr/lib/python3.12/site-packages/cockpit/channel.py", line 318, in send_data
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:     return self.send_text(self.__decode_frame(data))
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:                           ^^^^^^^^^^^^^^^^^^^^^^^^^
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:   File "/usr/lib/python3.12/site-packages/cockpit/channel.py", line 227, in __decode_frame
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:     return self.decoder.decode(data, final=final)
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]:   File "<frozen codecs>", line 322, in decode
Jul 22 16:30:14 hw-ovf21 cockpit-ws[2879]: UnicodeDecodeError: 'utf-8' codec can't decode byte 0xbd in position 6: invalid start byte
@jciesla jciesla added the bug label Jul 22, 2024
@jelly
Copy link
Member

jelly commented Jul 26, 2024

Thanks for the bug report, this is fairly easy to reproduce:

my_bytes = 'hello ÿ'.encode('utf-16')
open('foo.txt', 'wb').write(my_bytes)

Then in cockpit, cat foo.txt and see errors in journal.

@allisonkarlitskaya are you interested in fixing this?

@allisonkarlitskaya
Copy link
Member

Possibly related: #20628

@allisonkarlitskaya
Copy link
Member

@jciesla can you please elaborate on "latest versions"? Is there a known version of Cockpit that this definitely does not occur with?

@allisonkarlitskaya
Copy link
Member

This is caused by this line:

            self.decoder = codecs.getincrementaldecoder('utf-8')(errors='strict')

Having the decoder in strict mode is probably a bad idea. We ought to loosen that up to ignore/replace/surrogate errors. Replace, I guess.

@jciesla
Copy link
Author

jciesla commented Jul 26, 2024

Hello @allisonkarlitskaya, on version 300.3 the problem is definitely not present. I tested with versions 321 and 318. On version 318 the problem occurs but less often (no black display, only colors issue).

@martinpitt
Copy link
Member

martinpitt commented Jul 26, 2024

@allisonkarlitskaya No, no, no. This is the protocol level, strict errors was very deliberate. I'd hate to introduce "let's try to interpret junk as something interpretable" on that low level. This is a websocket, and it's UTF-8 (which is what triggered the whole commit 1b15dcb in the first place). In situations where we can't guarantee UTF-8 or anything else, IMHO the only correct option is binary: "raw".

@martinpitt
Copy link
Member

... and of course the bridge shouldn't crash unceremoniously like that, but close the channel with protocol-error.

@martinpitt martinpitt self-assigned this Aug 3, 2024
@martinpitt
Copy link
Member

martinpitt commented Aug 5, 2024

This is a lot more subtle than it appears. First of all, __decode_frame() already catches UnicodeDecodeError and is supposed to close the channel with protocol-error. This somehow doesn't happen.

I created a new unit test:

@pytest.mark.asyncio
async def test_fsread1_invalid_utf8(transport: MockTransport, tmp_path: Path) -> None:
    f = tmp_path / 'valid'
    data = b'hello' + bytes([0xFF, 0xFF]) + b'world'
    f.write_bytes(data)
    ch = await transport.check_open('fsread1', path=str(f))
    transport.send_done(ch)
    await transport.assert_msg('', command='close', channel=ch, problem='protocol-error')
    transport.send_close(ch)

but instead of failing, it receives exactly "helloworld" (without the broken binary junk) both on current main and with commit 1b15dcb reverted.

@martinpitt
Copy link
Member

Ah, this is because the fsread1 channel is outright evil!

                    if not self.is_binary:
                        data = data.replace(b'\0', b'').decode(errors='ignore').encode()

I.e. it silently gives you wrong information. This should be fixed separately, and I'll do this with the http channel.

martinpitt added a commit to martinpitt/cockpit that referenced this issue Aug 5, 2024
Unlike most other code paths, raising a ChannelError in data_received()
did not get translated to a proper close message. In particular,  this
caused an unhandled bridge exception in __decode_frame() when a text
channel encountered non-UTF8 data since commit 1b15dcb.

Side issue in cockpit-project#20791
martinpitt added a commit to martinpitt/cockpit that referenced this issue Aug 5, 2024
Terminals are not defined/required to be UTF-8 only. Sometimes people
cat binary data to the terminal, or possibly files in other encodings.
These cannot be sent through `TEXT` websocket channels, and will also
trigger the additional checks from commit 1b15dcb.

Use a binary channel to avoid all that. XTerm.js gets along with this
just fine, and it ignores invalid data.

With this you can now even `cat /bin/true` and get something moderately
sensible, instead of no output at all.

Fixes cockpit-project#20791
@martinpitt
Copy link
Member

Fixed in #20840

mvollmer pushed a commit that referenced this issue Aug 6, 2024
Unlike most other code paths, raising a ChannelError in data_received()
did not get translated to a proper close message. In particular,  this
caused an unhandled bridge exception in __decode_frame() when a text
channel encountered non-UTF8 data since commit 1b15dcb.

Side issue in #20791
mvollmer pushed a commit that referenced this issue Aug 6, 2024
Terminals are not defined/required to be UTF-8 only. Sometimes people
cat binary data to the terminal, or possibly files in other encodings.
These cannot be sent through `TEXT` websocket channels, and will also
trigger the additional checks from commit 1b15dcb.

Use a binary channel to avoid all that. XTerm.js gets along with this
just fine, and it ignores invalid data.

With this you can now even `cat /bin/true` and get something moderately
sensible, instead of no output at all.

Fixes #20791
@jciesla
Copy link
Author

jciesla commented Aug 6, 2024

Thank you for the fix. I tested on my side and all is now working correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants