Skip to content

GUACAMOLE-2270: Fix ABBA deadlock between guac_display_dup and guac_display_end_multiple_frames #665

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.

Sign up for GitHub

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

Open
masahji wants to merge 1 commit into
base: staging/1.6.1
Choose a base branch
from
Open

GUACAMOLE-2270: Fix ABBA deadlock between guac_display_dup and guac_display_end_multiple_frames #665

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions src/libguac/display.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,35 @@ void guac_display_dup(guac_display* display, guac_socket* socket) {

const guac_layer* layer = current->layer;

guac_rect layer_bounds;
guac_display_layer_get_bounds(current, &layer_bounds);
/* Read bounds directly from last_frame state (which is protected
* by display->last_frame.lock — already held above). Do NOT call
* guac_display_layer_get_bounds() here: that helper acquires
* display->pending_frame.lock and reads layer->pending_frame
* dimensions. Both are wrong here:
*
* 1. We are syncing the LAST FRAME state to a newly joined
* user, so layer->last_frame.width/height is the correct
* data source (every other field read in this loop already
* uses current->last_frame.*).
*
* 2. Acquiring pending_frame.lock from this code path creates
* an ABBA lock-order deadlock with
* guac_display_end_multiple_frames(), which acquires
* pending_frame.lock first and then last_frame.lock. With
* this function holding last_frame.lock and waiting for
* pending_frame.lock, while end_multiple_frames holds
* pending_frame.lock and waits for last_frame.lock, the
* pipeline wedges. Symptoms: guacd's VNC client thread,
* pending-user-promotion thread, and render loop all
* blocked on the display rwlocks; viewers see a black
* screen because no draws ever flow.
*/
Comment on lines +259 to +281

@mike-jumper mike-jumper May 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with these changes as they stand, but this comment is pretty heavy vs. the nature of the changes. The documentation for guac_display_layer_get_bounds() is pretty clear that its usage here would be a bug:

... The boundary stored will be the boundary of the current pending frame.

The full rundown of how/why this was noticed and what havoc is wrought when the wrong function is called isn't material to the fixed code. If you suspect that someone may erroneously use guac_display_layer_get_bounds() here again, a simple note that it can't be used here since it accesses the pending frame would be sufficient.

guac_rect layer_bounds = {
.left = 0,
.top = 0,
.right = current->last_frame.width,
.bottom = current->last_frame.height
};
Comment on lines +282 to +287

@necouchman necouchman May 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little uncertain that this is the best way to go about this - it seems to just duplicate the logic of the guac_display_layer_get_bounds() function for the sake of avoiding the lock issue. I looked through the code, and the guac_display_layer_get_bounds() function is only called in one other place (PFR_guac_display_plan_index_dirty_cells() in display-plan-search.c). I think it makes more sense to either:

  • Maintain the current function and adjust the locking to avoid the deadlock. This could mean removing the locks from the bounds function, or some other way of determining if the lock should be held or not.
  • Remove the function altogether and use this logic in any place (both of them, at the moment) where this gets called. I think this is likely not the way to go, I just don't see the point in removing half of the usages of this function and not actually addressing the deadlock in the function itself.

@mike-jumper @corentin-soriano @sschiffli Thoughts or input on this?

@masahji masahji May 1, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @necouchman, thanks for reviewing the PR. I'm not sure I would say that it's just duplicating the logic because I think it's hiding a real correctness issue that the PR description tried to call out in the "Secondary issue: wrong field source" section.

Basically the two call sites are operating on different frames:

Caller Operates on Reads
PFR_guac_display_plan_index_dirty_cells() pending state (uses &layer->pending_frame two lines later for the cell hash walk) pending_frame.{width, height}
guac_display_dup() last-flushed state (every other field in the loop reads current->last_frame.*: buffer, buffer_stride, parent, x, y, z, opacity, touches) should read last_frame.{width, height}

So guac_display_dup was never the right caller of guac_display_layer_get_bounds(): independent of the deadlock, it was reading the pending frame's dimensions while reading the last frame's pixel buffer. If a layer resize lands between frames, the size and the buffer disagree and the new user gets a corrupt sync. The deadlock was just a symptom that made us notice it.

What do you think about DRYing up the implementation by parameterizing the frame state into the guac_display_layer_get_bounds function.

Maybe something like:

void guac_display_layer_get_bounds(const guac_display_layer_state* state, guac_rect* bounds);

Each caller passes the frame they're already operating on, and is responsible for holding the corresponding lock:

// display-plan-search.c (already operates on pending state)
guac_display_layer_get_bounds(&layer->pending_frame, &layer_bounds);

// display.c, guac_display_dup (already holds last_frame.lock)
guac_display_layer_get_bounds(&current->last_frame, &layer_bounds);

I'd be happy to push a follow-up commit doing this if folks are good with this change. Or if you'd rather inline both call sites (option (b)), I can do that too.

@necouchman necouchman May 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@masahji Your proposed change makes sense to me, at least in concept - maybe go ahead and update the PR with those changes so that it can be reviewed in context?


int width = guac_rect_width(&layer_bounds);
int height = guac_rect_height(&layer_bounds);
Expand Down