Skip to content

Do not allow a null CEGLSync to reach doOnReadable #10976

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

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

Conversation

myQwil
Copy link
Contributor

@myQwil myQwil commented Jul 9, 2025

Describe your PR, what does it fix/add?

The function CEventLoopManager::doOnReadable is expecting its first parameter to be a non-null value, but CEGLSync::create can potentially return a nullptr.

If m_sync is null, and it reaches doOnReadable, Hyprland will crash.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

This is one way of addressing the problem, but there may be a better one. For example, maybe the null check should happen directly in doOnReadable. Here, I'm checking only where a null value might potentially reach the function.

Is it ready for merging, or does it need work?

It's ready for merging.

@gulafaran
Copy link
Contributor

you could use the isValid to catch more cases.

    if (!m_sync->isValid()) {
        Debug::log(ERR, "onFinishRender: failed to create CEGLSync, skipping.");
        return;
    }

question is if it should call onSyncFired directly then or return?

@myQwil
Copy link
Contributor Author

myQwil commented Jul 9, 2025

I'm not sure that'll work. CFileDescriptor::isValid() is also expecting a non-null value, plus doOnReadable already calls the isValid() function

@gulafaran
Copy link
Contributor

I'm not sure that'll work. CFileDescriptor::isValid() is also expecting a non-null value, plus doOnReadable already calls the isValid() function

oh lol the CEGLSync::isValid() is rather broken indeed, if the creation fails nullptr is returned. and isValid wont work.

@myQwil
Copy link
Contributor Author

myQwil commented Jul 9, 2025

Actually, never mind. I think the problem is m_sync->fd().duplicate(), so it never even enters the function doOnReadable. Instead, it crashes while trying to perform fd().duplicate()

@gulafaran
Copy link
Contributor

Actually, never mind. I think the problem is m_sync->fd().duplicate(), so it never even enters the function doOnReadable. Instead, it crashes while trying to perform fd().duplicate()

when is this crashing?

@gulafaran
Copy link
Contributor

ah renderer does this,

if (eglSync && eglSync->isValid()) {
so do something similiar i guess?

@myQwil
Copy link
Contributor Author

myQwil commented Jul 9, 2025

when is this crashing?

When trying to run a debug build of Hyprland, it was crashing while starting up. It has something to do with eglCreateSyncKHR failing.

@gulafaran
Copy link
Contributor

when is this crashing?

I'm trying to run a debug build of Hyprland and it crashes while starting up. It has to do with eglCreateSyncKHR failing.

void CMonitorFrameScheduler::onFinishRender() {
    m_sync = CEGLSync::create(); // this destroys the old sync

    if(m_sync && m_sync->isValid()) {
        g_pEventLoopManager->doOnReadable(m_sync->fd().duplicate(), [this, mon = m_monitor] {
            if (!m_monitor) // might've gotten destroyed
                return;
            onSyncFired();
        });
    }
    else
        Debug::log(ERR, "onFinishRender: failed to create CEGLSync, skipping.");
}

now the question still remains if at the end it should just call onSyncFired() anyways.
@vaxerski

@vaxerski
Copy link
Member

vaxerski commented Jul 9, 2025

if we can't sync, we should not try to triple buffer, as it won't work. Entire pipeline should be skipped and fallback to standard frame sync should be used

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

Successfully merging this pull request may close these issues.

3 participants