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

[Linux] Mumble should hint towards the need of ptrace_scope for positional audio #5711

Open
vimpostor opened this issue May 31, 2022 · 5 comments
Labels
feature-request This issue or PR deals with a new feature good first issue Good for first-time contributors positional audio

Comments

@vimpostor
Copy link
Contributor

vimpostor commented May 31, 2022

Context

Right now, on pretty much all reasonable distributions, the ptrace_scope is set to 1 ("restricted ptrace"), which means that positional audio plugins that require peeking into the game's memory pages, can't do that (unless theoretically mumble is a parent process of the game's process, but that is never the case).

Users need to set the ptrace_scope to 0 ("classic ptrace permissions"), so that those plugins can work. This can easily be done with the following command:

sudo tee /proc/sys/kernel/yama/ptrace_scope <<< 0

Description

It is bad UX for Mumble to just expect that the user knows about ptrace_scope. The user might not know at all about ptrace and may not understand why positional audio is not working.

There are a few solutions possible:

  • Mumble could add a small subtext in the related Settings pages (Audio output positional audio and Plugin page) that explains that some plugins may need this ptrace_scope
  • One could add another permission plugin error type to Mumble's enum, which then could be returned by positional audio plugins when the system call into the game's memory page fails (Mumble should then show a warning to the user)
  • Plugins could state themself beforehand that they require certain permissions, and Mumble could then show them in the Settings -> Plugins list directly next to the plugin with maybe a little warning icon

Mumble component

Client

OS-specific?

Yes, Linux

My opinion

I am in favor of the second solution, that is plugins should return a special permission error and Mumble could then show a warning.

@vimpostor vimpostor added feature-request This issue or PR deals with a new feature triage This issue is waiting to be triaged by one of the project members labels May 31, 2022
@Krzmbrzl
Copy link
Member

Plugins could just use the Mumble API to inform the user about the problem themselves. Just use the log function to write text to the chat console (where the user is expecting info about a plugin linking anyway).
We could also add a new API function for allowing the plugin to show a message as a popup instead of writing it to the chat console.

@vimpostor
Copy link
Contributor Author

Just use the log function to write text to the chat console (where the user is expecting info about a plugin linking anyway).

But that would end up in a mess, if we would have to duplicate the same log message in every plugin. I think Mumble could also test the value of /proc/sys/kernel/yama/ptrace_scope to show a better message, so that would end up in even more duplicated code.

@davidebeatrici
Copy link
Member

There's a problem: when the game runs through firejail, you still cannot access the memory even if ptrace_scope is set to 0 (default on Debian).

See #4506.

@Krzmbrzl
Copy link
Member

What itches me a bit about doing this in Mumble itself is that this would shift work that should really be the plugin's responsibility (after all it is the only authority to actually know whether this is required) to Mumble. Remember that the with the new plugin framework, not all plugins are positional data plugins and not even all of those require to peek into a process's memory.

Admittedly though, the vast majority of plugins that currently exist are positional data ones that do in fact require memory peeking. But still, this feels like it was overstepping a responsibility separation...

I guess we could just create a base class for positional data plugins that contains the logic to handle the check and the potential notification of the user and then simply let every other positional data plugin inherit from that. This way we'd still only have to maintain a single implementation but still the task is performed on the plugin's side. 🤔

@davidebeatrici
Copy link
Member

davidebeatrici commented May 31, 2022

Good idea, something like MemReadPermCheck.

Alternatively, we can just implement the check into ProcessBase.

@Krzmbrzl Krzmbrzl added positional audio good first issue Good for first-time contributors and removed triage This issue is waiting to be triaged by one of the project members labels Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue or PR deals with a new feature good first issue Good for first-time contributors positional audio
Projects
None yet
Development

No branches or pull requests

3 participants