sass / dart-sass

The reference implementation of Sass, written in Dart.
https://sass-lang.com/dart-sass
MIT License
3.9k stars 352 forks source link

`@mixin` recurrence causes OOM when using `sass` package and `compileStringAsync` api #2344

Closed hi-ogawa closed 1 day ago

hi-ogawa commented 3 days ago

This is originally reported on Vite https://github.com/vitejs/vite/issues/18091 and I made a minimal reproduction here https://github.com/hi-ogawa/reproductions/tree/main/vite-18091-sass-recurrence-oom

For the following code, sass-embedded gets to Internal compiler error: Stack Overflow quickly while sass package causes heap OOM.

// repro.mjs
async function main() {
    const sass =
        process.argv[2] === "sass"
            ? await import("sass")
            : await import("sass-embedded");
    const code = `
        @mixin test {
            @include test();
        }
        body {
            @include test();
        }
    `;
    const res = await sass.compileStringAsync(code, {
        url: new URL("file:///test.scss"),
    });
    console.log(res);
}

main();
$ node repro.mjs sass-embedded
Internal compiler error: Stack Overflow
dart:collection-patch/compact_hash.dart                _LinkedHashMapMixin._getValueOrData
package:sass/src/environment.dart                      Environment.getMixin
...

$ node repro.mjs sass
<--- Last few GCs --->

[21020:0x111e91d0]    19073 ms: Mark-Compact 4046.0 (4132.5) -> 4043.2 (4132.2) MB, 2144.83 / 0.00 ms  (average mu = 0.166, current mu = 0.042) allocation failure; scavenge might not succeed
[21020:0x111e91d0]    21670 ms: Mark-Compact 4059.2 (4132.2) -> 4056.4 (4159.2) MB, 2587.52 / 0.00 ms  (average mu = 0.080, current mu = 0.004) allocation failure; scavenge might not succeed

<--- JS stacktrace --->

FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory
----- Native stack trace -----
...
hi-ogawa commented 3 days ago

I forgot to check past issues and just noticed that this is probably a duplicate of https://github.com/sass/dart-sass/issues/1243.

I raised an issue here as I thought it's a bit odd that one is stack overflow and the other is heap OOM, but I guess it's an error either way, so not sure if this is really a bug. Feel free to triage as you see fit.

ntkme commented 2 days ago

This is working as expected - infinite recursion will cause stack overflow or out of memory, which is expected in any programming language. The reason that you are seeing different errors is that dart runtime and node runtime have different stack limit and memory limit, whichever runs out first would throw an error.


On the other hand, it is possible to run static analysis and perhaps warn about any possible unconditional recursion, but that probably out of scope for now.

nex3 commented 1 day ago

We could manually track the stack length to try to consistently throw a stack overflow error, but that would also unavoidably put a harder limit on legitimate recursion than the underlying machine can support, which isn't necessarily a good thing. I think leaving this as-is is best. Sass is a programming language, and if you make it loop infinitely it's going to fail.