GFX connection reliably crashes with SIGABRT #2968
Comments
|
Yup, I will be able to take a look tomorrow night (~7 PM EST on 2/27 and onwards) My first guess is this could be some sort of memory limit that we're hitting. |
|
I can reproduce this with a Windows VM with a 4K screen size (even though it doesn't fit on my main monitor!) Stack trace is:- I'll try running with valgrind and see if I can pick up the initial memory violation |
|
I can see what's happening and I have a fix, but it will need @Nexarian and/or @jsorg71 to work out why it works. I think we need to do this. valgrind trace is :- The memory buffer which isn't big enough is allocated at xrdp_encoder.c:669:- Lines 659 to 670 in c3cb855 and Lines 190 to 195 in c3cb855 I've done some playing around, and if I set I'm unable to comment further as I don't understand the 'magic number' comment. |
|
Thanks @matt335672. It makes sense it happens only 4k geometry with a big screen update. |
|
It looks like I added it in sha1 966a819 in the origin egfx branch in 09/20/2020. I don't know where I got that and can't find any limit in MS-RDPEGFX. librfxcodec should not crash though, rfx_encode_diff_rlgr1 should be checking the out buffer. I'll check on that. |
|
I called it a |
|
Let's increase it to enough size for 4K dual monitor (maybe 12 * 1024 * 1024?). |
|
@metalefty I think it only needs to be as big as the biggest monitor. The shared memory and the encoder work on a monitor at a time. If there is no monitors and a huge desktop, it's like you say. |
|
However, if there are no monitors and a huge desktop (e.g. if using VNC) we don't use the encoder. So I think this problem only occurs for GFX. |
|
Sorry - I mean GFX with XUP. |
|
Jay, thanks for the info. What about assuming a 16k x 16k per monitor as a maximum size for the moment then? It is the maximum size that xrandr (xorgxrdp?) reports. |
|
16k x 16k gives us a value of 256Mb. That's assuming that @metalefty's test program is producing worst case sizes, and it may not be. We can also set the size based on the largest monitor:- --- a/xrdp/xrdp_encoder.c
+++ b/xrdp/xrdp_encoder.c
@@ -85,6 +85,40 @@ xrdp_enc_data_done_destructor(void *item, void *closure)
g_free(enc_done);
}
+/*****************************************************************************/
+static unsigned int
+get_largest_monitor_pixels(struct xrdp_mm *mm)
+{
+ int max_pixels;
+
+ struct xrdp_client_info *client_info = mm->wm->client_info;
+ struct display_size_description *display_sizes;
+ display_sizes = &client_info->display_sizes;
+
+ if (display_sizes->monitorCount < 1)
+ {
+ max_pixels = display_sizes->session_width *
+ display_sizes->session_height;
+ }
+ else
+ {
+ max_pixels = 0;
+ struct monitor_info *minfo = display_sizes->minfo;
+ int i;
+ for (i = 0 ; i < display_sizes->monitorCount; ++i)
+ {
+ unsigned int pixels = (minfo[i].right + 1) - minfo[i].left;
+ pixels *= (minfo[i].bottom + 1) - minfo[i].top;
+ if (pixels > max_pixels)
+ {
+ max_pixels = pixels;
+ }
+ }
+ }
+
+ return max_pixels;
+}
+
/*****************************************************************************/
struct xrdp_encoder *
xrdp_encoder_create(struct xrdp_mm *mm)
@@ -189,9 +223,12 @@ xrdp_encoder_create(struct xrdp_mm *mm)
self->xrdp_encoder_term = g_create_wait_obj(buf);
if (client_info->gfx)
{
- // Magic numbers... Why?
+ // Assume compressor needs to cope with largest monitor with
+ // ineffective compression
self->frames_in_flight = 2;
- self->max_compressed_bytes = 3145728;
+ self->max_compressed_bytes = get_largest_monitor_pixels(mm) * 4;
+ LOG_DEVEL(LOG_LEVEL_INFO, "Using %d max_compressed_bytes for encoder",
+ self->max_compressed_bytes);
}
else
{That gives us 32Mb for a 4K monitor. Also, we're allocating What so you think @jsorg71 ? |
|
Ah, that looks best. Didn't test but that's the idea. |
|
Wow, 256M per monitor per user is huge and wasteful. That looks best to me, too. |
|
I'll put a PR together in the next hour or so. Jay - can you look at whether we need to test the buffer while we're writing into it? |
|
@matt335672 yes, sure I am already. In fact I'm confused on why it crashes. I'm planning to set max_compressed_bytes really low and debug what's happening. I'll create another issue for this so we can close this one. |
|
Reopened as it needs more investigation. |
|
This is taking longer than expected to fix. The way RFX works is that 'tiles_written' is returned and if that is not the whole tile set, it's called again. |
|
To my surprise, what I said last is not true. It looks like as long as all the tiles are send before the end frame we're OK. I wish there was documentation on how the works. |
|
should be fixed with #3013 and neutrinolabs/librfxcodec#61 |
|
That's great news @jsorg71. I'll test this on my own setup tomorrow (UK time). |
|
This one can be closed now. |
This issue is likely to be identical to the third issue I reported in #2942 (comment).
Testing the latest v0.10 branch c3cb855 on AlmaLinux 9.3.
Here's a simple program to reproduce: https://gist.github.com/metalefty/99b688ec67e5debb4d662fb6a12fc069
./a.out 200 300When trying to reconnect, xrdp process crashes SIGABRT as soon as reconnected because the same screen will be drawn again. If resize happens when reconnecting (reconnect from a different size), it gets back working. As far as I tested, this issue occurs when the session size is 4k. I can't reproduce this issue with 2560x1600 geometry. It is a smaller size than 3840x2160. It doesn't crash when the cols are less than 1/2 of the total screen width.
On FreeBSD, xrdp process doesn't crash but I get a black blank screen instead. As same as AlmaLinux, it gets back to working when I make resizing happen (moving mstsc to other monitors). It is something related to the GFX encoder.
I also tried v0.10 + cherry-pick #2962 but it doesn't affect this issue.
@Nexarian I remember you have a 4k monitor. Can you confirm if you can reproduce this issue?
The text was updated successfully, but these errors were encountered: