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

SDL_FindHashInTable() is not read-only #8391

Closed
icculus opened this issue Oct 13, 2023 · 2 comments
Closed

SDL_FindHashInTable() is not read-only #8391

icculus opened this issue Oct 13, 2023 · 2 comments
Assignees
Milestone

Comments

@icculus
Copy link
Collaborator

icculus commented Oct 13, 2023

This pattern is in SDL_properties.c in several places:

    if (SDL_LockRWLockForReading(SDL_properties_lock) == 0) {
        SDL_FindInHashTable(SDL_properties, (const void *)(uintptr_t)props, (const void **)&properties);
        SDL_UnlockRWLock(SDL_properties_lock);
    }

The problem is that SDL_FindHashInTable() will modify the hash...

            /* Matched! Move to the front of list for faster lookup next time.
               (stackable tables have to remain in the same order, though!) */
            if ((!table->stackable) && (prev != NULL)) {
                SDL_assert(prev->next == i);
                prev->next = i->next;
                i->next = table->table[hash];
                table->table[hash] = i;
            }

...and if two things have a read lock and come through here at the same time, it'll corrupt the hashtable.

So we should decide if we want to either move SDL_properties_lock to a normal mutex, or remove the optimization in SDL_FindInHashTable. I guess it's a question of which is more likely: multiple lookups of the same thing, or multiple threads looking stuff up in parallel.

If we remove the optimization, we should also move SDL_Properties::lock over to a RWlock.

@icculus icculus added this to the 3.2.0 milestone Oct 13, 2023
@icculus
Copy link
Collaborator Author

icculus commented Oct 13, 2023

(I'm inclined to say remove the optimization, so multiple threads can read from a hashtable simultaneously without using a lock at all when the hash is otherwise static...and also because this is a surprising implementation detail of the hashtable one wouldn't guess from the interface.)

@icculus icculus self-assigned this Oct 14, 2023
@icculus
Copy link
Collaborator Author

icculus commented Oct 14, 2023

Talked myself into the right solution eventually. :)

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

No branches or pull requests

1 participant