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.

Sign up for GitHub

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

Jump to bottom

Fix off by 1 issue in when relying on keySystems[].maxSessionCacheSize #1565

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Fix off by 1 issue in when relying on keySystems[].maxSessionCacheSize #1565

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
608ecbb
Fix off by 1 issue in when relying on `keySystems[].maxSessionCacheSize`
peaBerberian Oct 4, 2024
04d66cc
fix: clean old loaded session so the limit is correctly respected
Florent-Bouisset Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/main_thread/decrypt/create_or_load_session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,12 @@ export default async function createOrLoadSession(
}
}

await cleanOldLoadedSessions(loadedSessionsStore, maxSessionCacheSize);
await cleanOldLoadedSessions(
loadedSessionsStore,
// Account for the next session we will be creating
// Note that `maxSessionCacheSize < 0 has special semantic (no limit)`
maxSessionCacheSize <= 0 ? maxSessionCacheSize : maxSessionCacheSize - 1,
);
if (cancelSignal.cancellationError !== null) {
throw cancelSignal.cancellationError; // stop here if cancelled since
}
Expand Down
2 changes: 1 addition & 1 deletion src/main_thread/decrypt/utils/clean_old_loaded_sessions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export default async function cleanOldLoadedSessions(
loadedSessionsStore: LoadedSessionsStore,
limit: number,
): Promise<void> {
if (limit < 0 || limit >= loadedSessionsStore.getLength()) {
if (limit < 0 || limit > loadedSessionsStore.getLength()) {
Copy link
Collaborator

@Florent-Bouisset Florent-Bouisset Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure this is right:
If maxSessionCacheSize was set to 2 in the API, then limit here is equal to maxSessionCacheSize - 1 so limit is 1.

So before creating second session loadedSessionsStore.getLength() is 1.
Then 1 > 1 returns false and we clean the old sessions,
thus we should be able to have 2 sessions!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems to me that you're right though we wouldn't be closing anything thanks to:

const toDelete = entries.length - limit;

I'm not sure of what I was thinking here.

Can you fix it on your side? It's hard to develop in my current setup.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I have changed it, I removed the change on this file.
Basically this file was already fine, and behaves like cleanOldStoredPersistentInfo.

The only change needed is create_or_load_session.ts, that have the logic of setting the limit minus 1 because it's about to create a session.

The equivalent is already done for persistent sessions in the content decryptor:

cleanOldStoredPersistentInfo(
  persistentSessionsStore,
  EME_MAX_STORED_PERSISTENT_SESSION_INFORMATION - 1,
);

return;
}
log.info("DRM: LSS cache limit exceeded", limit, loadedSessionsStore.getLength());
Expand Down
Loading