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 writeContents argument function #6240

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

Comments

@erights
Copy link
Member

erights commented Sep 16, 2022

atomicWriteInRoot(baseName, tmpGzPath =>
filter(tmpSnapPath, createGzip(), tmpGzPath, { flush: true }),
),

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

        // The `await` within `atomicWriteInRoot` above is only safe
        // when the second `writeContents` argument,
        // in all calls to `atomicWriteInRoot`, is a function whose calls cannot
        // synchronously throw. Even though this arrow function
        // seems to only call `filter`, which is a local async function and
        // therefore safe by itself, this arrow function also calls
        // `createGzip()`, which we cannot locally verify does not
        // synchronously throw. Thus, we made the arrow function async,
        // so that it cannot throw regardless. This seems to be consistent
        // with its purpose, and with the likely assumptions the program
        // author made when placing it here.
        //
        atomicWriteInRoot(baseName, async tmpGzPath =>
          filter(tmpSnapPath, createGzip(), tmpGzPath, { flush: true }),
        ),

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 @dckc as the ones who should probably investigate this, so I'm assigning to them as well. Feel free to reassign as appropriate of course.

@erights erights added bug Something isn't working security labels Sep 16, 2022
@dckc
Copy link
Member

dckc commented Sep 16, 2022

@gibson042 is working on #6225 , which seems to overlap. Perhaps that's an opportunity to refactor/rewrite this code.

@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.

5 participants