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

options/m_config_frontend: don't allow recursive profile inclusion #15943

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kasper93
Copy link
Contributor

@kasper93 kasper93 commented Feb 23, 2025

It is easy to craft a config file that loads a profile recursively. To prevent this, not only check the depth of the current inclusion but also ignore already-seen profile names.

It is easy to craft a config file that loads a profile recursively. To
prevent this, not only check the depth of the current inclusion but also
ignore already-seen profile names.
Copy link

Download the artifacts for this pull request:

Windows
macOS

@Dudemanguy
Copy link
Member

It is easy to craft a config file that loads a profile recursively.

I thought this was a feature? It looks like this commit just avoids duplicate profile names though or am I misunderstanding it.

@kasper93
Copy link
Contributor Author

It is easy to craft a config file that loads a profile recursively.

I thought this was a feature? It looks like this commit just avoids duplicate profile names though or am I misunderstanding it.

After this patch mpv will not load already loaded profiles. I see no reason why we should allow recursive profiles load.

Consider this example config:

profile=e
[e]
profile=e
profile=e

@Dudemanguy
Copy link
Member

There's references to recursive profiles in the Conditional auto profiles section in the manual though. I mean I really don't mind either way but the change, if any, should be noted.

@Dudemanguy
Copy link
Member

Just for the the public record, I was misled by the wording in the manual. It's not really recursive. It's just that you can refer to another profile in the profile-cond. The PR has no effect and we can always update the language in the docs later.

@kasper93
Copy link
Contributor Author

Profiles are loaded recursively, this commit guards that we don't try to load already seen ones which would result in infinite recursion. And the current limit on include depth is not guarding against that fully as shown with example before.

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