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

R4.1: Support new GUI protocol version with flexible MSG_WINDOW_DUMP #21

Merged
merged 4 commits into from
Mar 2, 2019

Conversation

HW42
Copy link
Contributor

@HW42 HW42 commented Feb 16, 2018

This replaces the now deprecated MSG_MFNDUMP. For now the there's only
MSG_WINDOW_DUMP_TYPE_GRANT_REFS which uses grant references instead of
plain mfns as before.

@HW42
Copy link
Contributor Author

HW42 commented Feb 16, 2018

Depends on QubesOS/qubes-gui-common#2

@HW42 HW42 changed the title Support new GUI protocol version with flexible MSG_WINDOW_DUMP R4.1: Support new GUI protocol version with flexible MSG_WINDOW_DUMP Feb 16, 2018
@HW42
Copy link
Contributor Author

HW42 commented Feb 16, 2018

The Travis builds fails because it misses the new header from QubesOS/qubes-gui-common#2 (same for QubesOS/qubes-gui-agent-linux#34)

shmoverride/shmoverride.c Outdated Show resolved Hide resolved
shmoverride/shmoverride.c Outdated Show resolved Hide resolved
shmoverride/shmoverride.c Show resolved Hide resolved
@HW42
Copy link
Contributor Author

HW42 commented Feb 20, 2018

Do you prefer followup commits for improvements or replacing the existing commit with a improved one? I prefer the later since it leads to a clean history but it makes reviewing harder since you need to track old versions yourself.

@marmarek
Copy link
Member

I'm ok with either.

marmarek and others added 4 commits April 21, 2018 02:53
The previous fix properly handled only the case when no MSG_SHMIMAGE was
sent, but vm_window->image was already initialized (MSG_MFNDUMP). When
there was no window image at all, function arguments were ignored and
later no actual window update was performed (because size of requested
update appeared to be 0).

This fixes 2a314ca "gui-daemon: force colorful frame even when VM have
not sent window image".

Reported-by: Christoffer Kugg Jerkeby <kugg@fripost.org>
Fix potential integer overflow resulting in negative untrusted_[xy],
after being verified for that.
This replaces the now deprecated MSG_MFNDUMP. For now the there's only
MSG_WINDOW_DUMP_TYPE_GRANT_REFS which uses grant references instead of
plain mfns as before.
&shm_args, &shm_args_len);

if (g->invisible)
return;
Copy link
Member

Choose a reason for hiding this comment

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

shm_args is leaked.


// Something failed. Most likely the other domain already destroyed the
// buffer and thereby invalidated the refs. So create a dummy buffer. This
// mapping will probably be unmapped very soon anyway.
Copy link
Member

Choose a reason for hiding this comment

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

Why return dummy buffer, instead of a failure (NULL)? How grant refs version is special here?

Copy link
Member

Choose a reason for hiding this comment

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

It's about avoiding gui-daemon crash:

ErrorHandler: BadAccess (attempt to access private resource denied)
                 Major opcode: 130 (MIT-SHM)
                 Minor opcode: 1 (X_ShmAttach)
                 ResourceID:   0x1f5
                 Failed serial number:  101
                 Current serial number: 102

And from shmoverride:

gnttab: error: mmap failed: Invalid argument

I'm looking into more graceful handling of this error. I still think allocating (potentially large) memory buffer just to mute an error is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

And BTW this isn't specific to grant refs, it can also happen with old method: QubesOS/qubes-issues#4071

Copy link
Member

Choose a reason for hiding this comment

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

The problem with this is XShmAttach still reports success, but then the error is reported during subsequent XSync call. But since we need that XSync call anyway and I get detailed error info, I can correlate those two things (using global variable :/). This will fix QubesOS/qubes-issues#4071 too.

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

Successfully merging this pull request may close these issues.

2 participants