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

Increase buffer size in JS_ThrowError2 #795

Closed
wants to merge 1 commit into from

Conversation

ABBAPOH
Copy link
Contributor

@ABBAPOH ABBAPOH commented Jan 6, 2025

External apps may produce long error messages when using JS as an engine. Those messages might include long file paths.

256 characters is way to little for any meaningfull error
that includes backtrace.
@ABBAPOH ABBAPOH changed the title Inicrease buffer size in JS_ThrowError2 Increase buffer size in JS_ThrowError2 Jan 6, 2025
@saghul
Copy link
Contributor

saghul commented Jan 6, 2025

256 characters is way to little for any meaningfull error that includes backtrace.

But the generated backtrace does not count towards that number, am I missing something? Or are you crafting really long error messages including backtrces from other things?

@ABBAPOH
Copy link
Contributor Author

ABBAPOH commented Jan 6, 2025

We use the JSThrowError* functions when reporting errors from JS extensions written in C++.
These errors might include file paths, for example https://github.com/qbs/qbs/blob/master/src/lib/corelib/jsextensions/file.cpp#L147-L153
I will rewrite PR message.

@saghul
Copy link
Contributor

saghul commented Jan 6, 2025

Ah I see!

@bnoordhuis
Copy link
Contributor

I'm slightly worried about stack-allocating such a big buffer. We don't do that anywhere else, I don't think.

Some users run quickjs on very small stacks (like musl threads) and this might be enough to push it over the limit.

It'd be slightly less bad if it was inside a leaf function (because then a stack overflow trap clearly points to the offending function) but it isn't.

@ABBAPOH
Copy link
Contributor Author

ABBAPOH commented Jan 7, 2025

What if check for musl? And use smaller buffer, like 1K.
I assume simple #if defined(MUSL) would work?

@saghul
Copy link
Contributor

saghul commented Jan 7, 2025

Might be better to add an API where you can pass an externally created buffer?

Creating the JS string will copy it anyway, this way we push the responsibility to the user :-)

@ABBAPOH
Copy link
Contributor Author

ABBAPOH commented Jan 8, 2025

Hm, it seems I was wrong and we don't actually use this buffer - we simply throw message created on the C++ side.
I'll abandon this for now.

@ABBAPOH ABBAPOH closed this Jan 8, 2025
@ABBAPOH ABBAPOH deleted the buffer branch January 11, 2025 21:41
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.

3 participants