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

Await safety not yet fixed in swing-store/src/snapStore.js for sync call #6239

Closed
erights opened this issue Sep 16, 2022 · 2 comments
Closed
Assignees
Labels
bug Something isn't working security vaults_triage DO NOT USE
Milestone

Comments

@erights
Copy link
Member

erights commented Sep 16, 2022

await destination.sync();

The triage currently at #6219 would fix it, with the explanation and code

        // This nested await has been made SAFER by "IIAFE". Note that
        // doing so does *not* postpone when the `sync()` happens,
        // which would be especially scary for something named `sync()`.
        // Rather, it potentially postpones when the `close()`s happen
        // in case `sync()` throws. With the IIAFE, if this line is
        // executed, then the `close()`s would always happen in a later turn.
        //
        // But this nested await is not actually safe because it is nested in
        // an `if`, the other branch of which proceeds synchronously. The
        // `if` is not terminal because of the stateful `close()`s afterward.
        // On the else branch, those `close()`s would happen synchronously.
        // eslint-disable-next-line @jessie.js/no-nested-await
        await (async () => destination.sync())();

Most of #6219 consists of commentary or cosmetic code changes, and so can be postponed until after PS0. But this is a safety fix. Whether we should cherry pick it, to get it in before PS0 depends on what the consequences are of not fixing it.

Assigning myself since I have the fix done in #6219 and would do the cherry picking if we decide to do that.

Git blame shows @gibson042 and @mhofman as the ones who should probably investigate this, so I'm assigning to them as well. Feel free to reassign as appropriate of course.

Also assigning @dtribble since he commented on this one and suggested a different fix.

@warner
Copy link
Member

warner commented Jan 24, 2023

That code has been deleted as part of the SQLite conversion (#6742). I don't see any conditional or nested awaits left in the current version of that file (https://github.com/Agoric/agoric-sdk/blob/b2a96d42cb81f84f48573cdfba7fa6916f2ea998/packages/swing-store/src/snapStore.js), so I'm guessing we can close this, but @erights maybe you could do another pass through it? I know you see things that I don't.

@erights
Copy link
Member Author

erights commented Apr 15, 2023

With the simpler await safety rule, this issue is obsolete. Closing.

@erights erights closed this as completed Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security vaults_triage DO NOT USE
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants