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

resolve bytes vs strings conversion issues #3229

Closed
totaam opened this issue Aug 1, 2021 · 43 comments
Closed

resolve bytes vs strings conversion issues #3229

totaam opened this issue Aug 1, 2021 · 43 comments
Labels
enhancement New feature or request network

Comments

@totaam
Copy link
Collaborator

totaam commented Aug 1, 2021

This change totaam/rencode@4509f95 would allow us to use rencode the way we always wanted to use it: strings come out as strings and bytes come out as bytes...
Deluge hit the same issue: https://dev.deluge-torrent.org/ticket/2216

Before this can happen:

  • fix all the issues where we wrongly assumed that bytes and strings are the same thing
  • release a new rencode version (and make the same changes to the javascript version: handle bytes with rencode xpra-html5#84)
  • add a toggle so we only enable the new "bytes" mode if the remote end also supports it
@totaam
Copy link
Collaborator Author

totaam commented Aug 1, 2021

Already:

  • encodings capabilities 63b4557
  • client handling for rgb_format 63c9121

Not sure why this existing bug only triggered now: b96aaee

totaam added a commit that referenced this issue Aug 3, 2021
@totaam
Copy link
Collaborator Author

totaam commented Aug 3, 2021

After experimenting with removing the lock used to make the non-cythonized version of rencode safe: https://github.com/totaam/rencode/tree/orig-nolock
It's pretty clear that the changes are just too intrusive and make the code a lot slower. (10000 times or more!)
In reality it would not be such a big problem because we could actually instantiate an encoder + decoder context per connection just once - and it is the instantiation that is costly.
Doing the same change to the cython version - the only version actually used - would be much harder.

A better approach is just to "fork" this code directly into the xpra code: rencodeplus.
Something I should have done a long time ago.

Supporting multiple connections with different capabilities is only really a problem for servers.
The html5 client has been updated to support bytes without too much trouble: Xpra-org/xpra-html5@1f83163

totaam added a commit that referenced this issue Aug 3, 2021
@totaam
Copy link
Collaborator Author

totaam commented Aug 4, 2021

@basilgello : as per the discussion in bb867cd, I had introduced a surprising bug in a last minute change...

And here's the cause:

def f(v):
    print(v)
> x={}
> x[f(0)] = f(1)
1
0

The expression is evaluated in a different order than I had expected...

@basilgello
Copy link
Contributor

Latest trunk (fixed). Server: xpra start --start=xeyes --daemon=no
Client: Xpra GTK3 client. On close of Xeyes window:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/xpra/client/gtk_base/gtk_client_window_base.py", line 2092, in do_delete_event
    self._client.window_close_event(self._id)
  File "/usr/lib/python3/dist-packages/xpra/client/mixins/window_manager.py", line 1128, in window_close_event
    matching_title_close = [x for x in TITLE_CLOSEEXIT if x and title.startswith(x)]
  File "/usr/lib/python3/dist-packages/xpra/client/mixins/window_manager.py", line 1128, in <listcomp>
    matching_title_close = [x for x in TITLE_CLOSEEXIT if x and title.startswith(x)]
TypeError: startswith first arg must be bytes or a tuple of bytes, not str

@basilgello
Copy link
Contributor

Also, Xpra system-wide proxy creates /run/user/$UID/xpra/$XPRA_SESSION directories as root… like:

$ ls -la /run/user/1000/xpra
загалом 0
drwx------ 3 test test  60 сер  4 12:16 .
drwx------ 9 test test 260 сер  4 12:15 ..
drwxr-x--- 2 root    root    140 сер  4 12:15 1

@basilgello
Copy link
Contributor

Finally, removing python3-rencode activates rencodeplus in Xpra GTK3 client but HTML5 client fails to draw anything except of window header:

сер 04 12:15:30 test xpra[19577]: Warning: ignoring unknown video encoders: none
сер 04 12:15:30 test xpra[19577]: Error: invalid codec name 'enc_none'
сер 04 12:15:30 test xpra[19577]: proxy video encoders: none

@basilgello
Copy link
Contributor

basilgello commented Aug 4, 2021

Xvfb-for-Xpra also do not gracefully shutdown on xpra server exit :( Same applies for dbus-daemon

Clear permission error on Xpra server shutdown:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/xpra/server/server_core.py", line 305, in clean_quit
    self.quit_worker()
  File "/usr/lib/python3/dist-packages/xpra/server/server_core.py", line 323, in quit_worker
    self.quit()
  File "/usr/lib/python3/dist-packages/xpra/server/server_core.py", line 350, in quit
    self.late_cleanup()
  File "/usr/lib/python3/dist-packages/xpra/x11/x11_server_base.py", line 120, in late_cleanup
    super().late_cleanup()
  File "/usr/lib/python3/dist-packages/xpra/server/gtk_server_base.py", line 76, in late_cleanup
    super().late_cleanup()
  File "/usr/lib/python3/dist-packages/xpra/server/server_core.py", line 455, in late_cleanup
    self.clean_session_files()
  File "/usr/lib/python3/dist-packages/xpra/server/server_core.py", line 459, in clean_session_files
    self.do_clean_session_files(*self.session_files)
  File "/usr/lib/python3/dist-packages/xpra/server/server_core.py", line 463, in do_clean_session_files
    clean_session_files(*filenames)
  File "/usr/lib/python3/dist-packages/xpra/scripts/server.py", line 393, in clean_session_files
    clean_session_files(*glob.glob(path))
  File "/usr/lib/python3/dist-packages/xpra/scripts/server.py", line 403, in clean_session_files
    rm_session_dir()
  File "/usr/lib/python3/dist-packages/xpra/scripts/server.py", line 377, in rm_session_dir
    if not os.listdir(session_dir):
PermissionError: [Errno 13] Відмовлено у доступі: '/run/user/1000/xpra/1'

@basilgello
Copy link
Contributor

Clear permission error on Xpra server shutdown:

I solved this by adding:

os.lchown(session_dir, uid, gid)

before https://github.com/Xpra-org/xpra/blob/master/xpra/scripts/server.py#L875

@totaam
Copy link
Collaborator Author

totaam commented Aug 4, 2021

TypeError: startswith first arg must be bytes or a tuple of bytes, not str

Fixed in c12bca9.

Also, Xpra system-wide proxy creates /run/user/$UID/xpra/$XPRA_SESSION directories as root…

That's #3217.

I solved this by adding:

I've done something almost identical in 119cc8a except it only chowns it if we've just created the directory.
I'm not sure that's right, but that's what we do everywhere else... so done for consistency.
One could argue that using a directory owned by another user is bad, but in that case chowning it is probably also bad?
And if we still fail, at least we should handle it more gracefully: 86c134e

HTML5 client fails to draw anything except of window header

@basilgello that's strange because the html5 client is what I used primarily for developing this new rencodeplus and doing so fixed a number of bugs: Xpra-org/xpra-html5#84
Works fine for me. 🤷‍♂️
Are you connecting via the proxy or something?

@basilgello
Copy link
Contributor

Are you connecting via the proxy or something?

Yes via proxy!

totaam added a commit that referenced this issue Aug 4, 2021
for now we may get either and we should not care,
convert to strings to make the code easier to read
@totaam
Copy link
Collaborator Author

totaam commented Aug 4, 2021

Yes via proxy!

Ah, yes: b20fefd fixes that.
Though there are still some warnings I will look into.

totaam added a commit that referenced this issue Aug 4, 2021
for now we may get either and we should not care,
convert to strings to make the code easier to read
@basilgello
Copy link
Contributor

Yeah, this forks! :D

totaam added a commit that referenced this issue Aug 5, 2021
(this is not compatible with the previous version)
@basilgello
Copy link
Contributor

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/xpra/client/client_base.py", line 1058, in call_handler
    handler(packet)
  File "/usr/lib/python3/dist-packages/xpra/client/mixins/window_manager.py", line 1099, in _process_configure_override_redirect
    window.move_resize(ax, ay, aw, ah, -1)
  File "/usr/lib/python3/dist-packages/xpra/client/gtk_base/gtk_client_window_base.py", line 2009, in move_resize
    x, y = self.adjusted_position(x, y)
  File "/usr/lib/python3/dist-packages/xpra/client/gtk_base/gtk_client_window_base.py", line 660, in adjusted_position
    if AWT_RECENTER and self.is_awt(self._metadata):
  File "/usr/lib/python3/dist-packages/xpra/client/gtk_base/gtk_client_window_base.py", line 543, in is_awt
    return wm_class and len(wm_class)==2 and wm_class[0].startswith("sun-awt-X11")
TypeError: startswith first arg must be bytes or a tuple of bytes, not str

@totaam
Copy link
Collaborator Author

totaam commented Aug 6, 2021

TypeError: startswith first arg must be bytes or a tuple of bytes, not str

@basilgello that's the same as: #3232 (comment)
Which should already be fixed by 123efb0 - is it not?

@basilgello
Copy link
Contributor

I still use master trunk from yesterday around 22:00 EET :) Let me build the trunk right now.

Also, two more issues spotted:

  • GTK3 client fails to forward audio at all. Before rencodeplus it at least worked with stutters.
  • Download Server Log menu is inactive (greyed out)

@totaam
Copy link
Collaborator Author

totaam commented Aug 6, 2021

As part of this work on rencode I found a DoS: totaam/rencode@f6254ab

Here's the exploit to cause the xpra server to go unresponsive and consume 100%CPU until it eventually reaches out-of-memory:

python -c "from xpra.net.header import pack_header, FLAGS_RENCODE; \
    print((pack_header(FLAGS_RENCODE, 0, 0, 3)+b';\x2f\x7f').decode())" \
    | nc 127.0.0.1 10000

Mitigations:

  • updated rencode RPMs are available in the stable repositories - DEB users will have to bug their maintainers
  • remove the cython accelerated rencode module - as the plain python implementation does not have this bug:
rm `python3 -c "from rencode import _rencode;print(_rencode.__file__)"`

The performance loss is not critical.

  • disable rencode using --packet-encoders=bencode: this may have undesirable side effects and may expose some bugs (some were fixed quite recently)

@basilgello
Copy link
Contributor

You should CVE it as you forked it from aresch I guess :)

@basilgello
Copy link
Contributor

Server -> Server Commands menu: Start New errors out:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/xpra/client/gtk_base/gtk_client_base.py", line 460, in show_start_new_command
    self.start_new_command = getStartNewCommand(run_command_cb,
  File "/usr/lib/python3/dist-packages/xpra/client/gtk_base/start_new_command.py", line 28, in getStartNewCommand
    _instance = StartNewCommand(run_callback, can_share, xdg_menu)
  File "/usr/lib/python3/dist-packages/xpra/client/gtk_base/start_new_command.py", line 60, in __init__
    self.category_combo.append_text(name.decode("utf-8"))
AttributeError: 'str' object has no attribute 'decode'

@basilgello
Copy link
Contributor

With latest html5 trunk I saw only a couple of glitches 👍 I am testing on rendered PDFs now to be sure HTML5 client scrolling is usable again.

@totaam
Copy link
Collaborator Author

totaam commented Aug 8, 2021

With latest html5 trunk I saw only a couple of glitches

Anything specific? I was going to release xpra-html5 version 4.3 today and xpra 4.2.2 immediately after.

@basilgello
Copy link
Contributor

Anything specific?

Well, not something reproducible in stable manner :) BTW, for me html5 client sound was totally broken regardless of xpra server side. Overload errors in JS types all the way. Now when recent Xpra has SoundPipeline broken (and I have yet to investigate why), I can not give you exact reproducer for html5 sound bug.

@basilgello
Copy link
Contributor

Stopping xpra via xpra stop :1 spews this:

Warning: 'rencodeplus' packet encoder was not found
 this build of xpra may be incomplete
server requested disconnect:
 server shutdown
xpra at :1 has exited.

That is strange because I have no python3-rencode package but use,rencodeplus. I guess this warning should pop if there are no both rencode or rencodeplus, ie only bencode is present

@totaam
Copy link
Collaborator Author

totaam commented Aug 8, 2021

BTW, for me html5 client sound was totally broken regardless of xpra server side

Is this a regression? Do you know the last version that worked for you?

Now when recent Xpra has SoundPipeline broken

I assume that's in master only, right? I can release 4.2.2 without worrying about this?

@basilgello
Copy link
Contributor

Is this a regression? Do you know the last version that worked for you?

TBH sound never worked for me within html5 client.

I assume that's in master only, right? I can release 4.2.2 without worrying about this?

I assume yes. Just check you can hear a Youtube video via html5 client there :)

@basilgello
Copy link
Contributor

Latest trunk of Xpra. GTK3 client again can play sound (choppy but still 👍 )

Server spews this:

2021-08-09 09:14:18,251 Error: invalid access from thread <Thread(parse, started daemon 140516473685760)>
  File "/usr/lib/python3.9/threading.py", line 912, in _bootstrap
    self._bootstrap_inner()
  File "/usr/lib/python3.9/threading.py", line 954, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.9/threading.py", line 892, in run
    self._target(*self._args, **self._kwargs)
  File "/usr/lib/python3/dist-packages/xpra/net/protocol.py", line 802, in _read_parse_thread_loop
    self.do_read_parse_thread_loop()
  File "/usr/lib/python3/dist-packages/xpra/net/protocol.py", line 1030, in do_read_parse_thread_loop
    self._process_packet_cb(self, packet)
  File "/usr/lib/python3/dist-packages/xpra/client/ui_client_base.py", line 823, in process_packet
    XpraClientBase.process_packet(self, proto, packet)
  File "/usr/lib/python3/dist-packages/xpra/client/client_base.py", line 1060, in process_packet
    call_handler()
  File "/usr/lib/python3/dist-packages/xpra/client/client_base.py", line 1057, in call_handler
    handler(packet)
  File "/usr/lib/python3/dist-packages/xpra/client/client_base.py", line 908, in _process_hello
    if not self.server_connection_established(caps):
  File "/usr/lib/python3/dist-packages/xpra/client/ui_client_base.py", line 419, in server_connection_established
    if not XpraClientBase.server_connection_established(self, caps):
  File "/usr/lib/python3/dist-packages/xpra/client/client_base.py", line 922, in server_connection_established
    if not self.parse_server_capabilities(caps):
  File "/usr/lib/python3/dist-packages/xpra/client/ui_client_base.py", line 428, in parse_server_capabilities
    if not cb.parse_server_capabilities(self, c):
  File "/usr/lib/python3/dist-packages/xpra/client/mixins/audio.py", line 208, in parse_server_capabilities
    self.start_sending_sound()
  File "/usr/lib/python3/dist-packages/xpra/client/mixins/audio.py", line 295, in start_sending_sound
    enabled = self.start_sound_source(device)
  File "/usr/lib/python3/dist-packages/xpra/client/mixins/audio.py", line 316, in start_sound_source
    ss = start_sending_sound(plugins, self.sound_source_plugin, device or self.microphone_device,
  File "/usr/lib/python3/dist-packages/xpra/sound/wrapper.py", line 375, in start_sending_sound
    plugin, options = parse_sound_source(plugins, sound_source_plugin, device, want_monitor_device, remote)
  File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 867, in parse_sound_source
    options = get_sound_source_options(simple_str, options_str, device, want_monitor_device, remote)
  File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 825, in get_sound_source_options
    options = defaults_fn(device, want_monitor_device, remote)
  File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 741, in get_pulse_source_defaults
    return get_pulse_defaults(device_name_match, want_monitor_device,
  File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 579, in get_pulse_defaults
    device = get_pulse_device(device_name_match, want_monitor_device, input_or_output, remote, env_device_name)
  File "/usr/lib/python3/dist-packages/xpra/sound/gstreamer_util.py", line 630, in get_pulse_device
    pa_id = get_pulse_id()
  File "/usr/lib/python3/dist-packages/xpra/sound/pulseaudio/pulseaudio_pactl_util.py", line 120, in get_pulse_id
    return get_pulse_id_x11_property()
  File "/usr/lib/python3/dist-packages/xpra/sound/pulseaudio/pulseaudio_common_util.py", line 59, in get_pulse_id_x11_property
    return get_x11_property("PULSE_ID")
  File "/usr/lib/python3/dist-packages/xpra/sound/pulseaudio/pulseaudio_common_util.py", line 32, in get_x11_property
    with xswallow:
  File "/usr/lib/python3/dist-packages/xpra/gtk_common/error.py", line 206, in __enter__
    trap.Xenter()
  File "/usr/lib/python3/dist-packages/xpra/gtk_common/error.py", line 100, in Xenter
    verify_main_thread()
  File "/usr/lib/python3/dist-packages/xpra/gtk_common/error.py", line 57, in verify_main_thread
    traceback.print_stack()

@totaam
Copy link
Collaborator Author

totaam commented Aug 9, 2021

Warning: 'rencodeplus' packet encoder was not found

I believe a6151c1 fixes this.

Server spews this:

That's a client error, forwarded to the server via remote logging.
You must have started your client with --microphone=on, right?
That's unrelated to this ticket and fixed in f7308d8.

@basilgello
Copy link
Contributor

basilgello commented Aug 16, 2021

master trunk of xpra + xpra-html5 built 15 minutes ago:

  • again network notofication shows byte represebtation instead of string (ie 12,23,…)
  • JPEG encoding is no longer smooth in HTML5 client - there are stucks and spinners with average network load spikes to ~6MB/sec

Keeping latest Xpra server but reverting HTML5 client to b28eadd resolves an issue

@totaam
Copy link
Collaborator Author

totaam commented Aug 17, 2021

again network notification shows byte represebtation instead of string
JPEG encoding is no longer smooth in HTML5 client

My guess is that this was before 094169d and therefore using rencode instead of rencodeplus.
I doubt that switching decoding to a worker is the problem (Xpra-org/xpra-html5@423f80a) but the new lz4 decoder (Xpra-org/xpra-html5@f507836) seems to have a problem with rencoded data. (strings vs bytes..)

@basilgello
Copy link
Contributor

The latest trunk of html5 client still does not resolve the broken JPEG decoding. I guess I have to bisect the bug again in a separate issue in html5

@totaam
Copy link
Collaborator Author

totaam commented Aug 18, 2021

@basilgello are you sure that you are using rencodeplus with the html5 client?
xpra info | grep client.connection.encoder

@basilgello
Copy link
Contributor

Yes, in both cases,it does use,rencodeplus:

$ xpra info :1 | grep client.connection.encoder
client.connection.encoder=rencodeplus

@basilgello
Copy link
Contributor

I tried to check which commit introduces the regression: lz4 removal or webp/jpeg decoding in separate worker.

The reference point which has smooth Youtube video playback in Firefox window on server while viewed on Chromium for Android client is b28eadd

Both issues (broken notification window and stuttering playback) are introduced with a0f50fd7121a246722666314e331210dfc6435df Adding separate lz4 decryptor makes no change. I am now going to backport lz4 decryptor removal omitting worker thread commits to check if lz4 internal decoder also adds to performance drops.

@basilgello
Copy link
Contributor

5704c7cf24c4c817ab1fc707f8a652968394f77f fixes notification pop-up but the main issue with stutter still persists… With b28eadd the network usage graph on my Android tab usually flows around ~300-1200kB/second, notification pop-ups are infrequent and literally no spinners. With separate worker commit and beyond, up to currently latest trunk, the playback freezes and network usage spikes to ~6mB/sec often accompanied by spinner. Should I debug network and draw calls?

@totaam
Copy link
Collaborator Author

totaam commented Aug 18, 2021

@basilgello Please create a separate issue on the https://github.com/Xpra-org/xpra-html5 issue tracker, as this has nothing to do with rencode (AFAICT) and the commit links are all broken.

The broken notification is already fixed: Xpra-org/xpra-html5@a23c611

totaam added a commit that referenced this issue Aug 31, 2021
always send strings as they are, bencode and rencode will encode them to utf8 bytes
rencodeplus will just preserve them

on the receiving side, if we get a string then there should be nothing to do (rencodeplus)
and if we get bytes then we need to decode as utf8
totaam added a commit that referenced this issue Aug 31, 2021
let the packet layer handle it, mostly. (still use net_utf8() to retrieve string values that may have been converted to bytes)
@totaam totaam closed this as completed Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request network
Projects
None yet
Development

No branches or pull requests

2 participants