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

fix 14540 #15498

Merged
merged 6 commits into from
Dec 2, 2024
Merged

fix 14540 #15498

merged 6 commits into from
Dec 2, 2024

Conversation

dylan-conway
Copy link
Member

@dylan-conway dylan-conway commented Nov 29, 2024

What does this PR do?

fixes #14540

Fixes crashes caused by invalid pointers copied from old lexer state.

How did you verify your code works?

Added a test that relies on std.ArrayList capacity growth

@robobun
Copy link

robobun commented Nov 29, 2024

@dylan-conway, your commit 37baaef has 12 failures in #7150:

  • test/js/bun/http/bun-serve-static.test.ts - timeout on 🐧 3.20 x64
  • test/js/node/test/parallel/worker-nested-uncaught.test.js - segmentation fault on 🐧 3.20 x64
  • test/js/node/test/parallel/worker-nested-uncaught.test.js - segmentation fault on 🐧 3.20 x64-baseline
  • test/integration/next-pages/test/dev-server.test.ts - 1 failing on 🐧 3.20 x64
  • test/integration/next-pages/test/dev-server.test.ts - 1 failing on 🐧 3.20 x64-baseline
  • test/cli/hot/hot.test.ts - timeout on 🐧 22.04 x64
  • test/cli/hot/hot.test.ts - timeout on 🐧 3.20 x64
  • test/cli/hot/hot.test.ts - timeout on 🐧 3.20 x64-baseline
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 3.20 aarch64
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 11 x64-baseline
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 22.04 x64-baseline
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 12 x64
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 22.04 x64
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 3.20 x64
  • test/js/node/test/parallel/fs-watch-recursive-linux-parallel-remove.test.js - 1 failing on 🐧 3.20 x64-baseline
  • test/v8/v8.test.ts - 22 failing on 🐧 3.20 aarch64
  • test/v8/v8.test.ts - 22 failing on 🐧 3.20 x64-baseline
  • test/v8/v8.test.ts - 22 failing on 🐧 3.20 x64
  • test/js/node/test/parallel/timers-ordering.test.js - 1 failing on 🐧 22.04 x64
  • test/js/node/child_process/child_process.test.ts - 1 failing on 🐧 3.20 aarch64
  • test/js/node/child_process/child_process.test.ts - 1 failing on 🐧 3.20 x64
  • test/js/node/child_process/child_process.test.ts - 1 failing on 🐧 3.20 x64-baseline
  • test/js/bun/http/serve.test.ts - SIGTRAP on 🐧 3.20 aarch64
  • test/js/bun/http/serve.test.ts - SIGILL on 🐧 3.20 x64
  • test/js/bun/http/serve.test.ts - SIGILL on 🐧 3.20 x64-baseline
  • test/js/web/fetch/fetch-leak.test.ts - 1 failing on 🪟 2019 x64
  • test/integration/next-pages/test/next-build.test.ts - 1 failing on 🍎 13 x64
  • test/integration/next-pages/test/next-build.test.ts - 1 failing on 🐧 22.04 x64-baseline
  • test/js/bun/shell/leak.test.ts - 1 failing on 🪟 2019 x64
  • @@ -273,6 +273,27 @@ fn NewLexer_(
    // }
    }

    pub fn restore(this: *LexerType, original: *const LexerType) void {
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    The length is not the main problem - the main problem is on array resize, the previous pointer is now invalid memory. This wouldn't fix it, as it reuses the memory.

    I think we would instead need to lazily clone it

    Copy link
    Member Author

    Choose a reason for hiding this comment

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

    it copies the possibly new pointer from the new lexer, and sets the length from the original lexer. This way we can ensure the pointers are valid, new values aren't used, and memory isn't wasted.

    doing this for temp_buffer_u16 is unnecessary though, it's length is set to 0 after each use so we can ignore it. Will update

    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    the pointer can become invalid memory though when its resized later. even if the ptr and length match up.

    @dylan-conway dylan-conway marked this pull request as ready for review December 2, 2024 10:00
    @dylan-conway dylan-conway merged commit 6d453be into main Dec 2, 2024
    65 of 67 checks passed
    @dylan-conway dylan-conway deleted the dylan/fix-14540 branch December 2, 2024 22:57
    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

    Successfully merging this pull request may close these issues.

    bun build crash
    3 participants