Switch from SpiderMonkey 1.7 to Duktape #208
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Author
|
Note that the Duktape used is 2.7 and built with default options. It could perhaps be made unvendored and rely on a system Duktape, but that seems hairy with the current makefile. |
Owner
|
@NattyNarwhal Sorry for super late review, but this looks great to me. Thanks so much. Do you want to send it for review? |
Author
|
I'd appreciate review; I'm uncertain about lifetimes myself, though it seems to be OK with the unit tests. Windows support is also untested, as is unvendoring Duktape for i.e. distro packaging. |
pacparser currently vendors SpiderMonkey 1.7, a JavaScript engine that predates the Obama presidency. There's been a ton of changes to JavaScript and best practices when it comes to security and portability, so using this old version of SM doesn't make sense anymore. People are [trivially able to write exploits][ancientmonkey] against this old version, and seeing as PAC files could come from untrusted networks, that doesn't seem like a wise decision. To replace it, I've used duktape, a popular compact and embeddable JS runtime. There are a lot, but duktape seems popular; for example, polkit switched from (newer) SpiderMonkey to duktape. The only change I've needed to make to JS code is that RegExps don't seem to be callable under duktape; they aren't under V8 either though, so this might have been a Mozilla-ism. The massively smaller codebase of duktape is hopefully better security and maintainability wise, but also results in much smaller binaries. For example, pactester goes from 1.5M to 687K on my system. Passes unit tests on macOS. Not tested on Linux/Windows yet. However, I'm not certain about i.e. string lifetimes with duktape. They didn't seem clear with SpiderMonkey either though; perhaps it'd be an opportunity to i.e. explicitly strdup them? [ancientmonkey]: https://blog.pspaul.de/posts/ancient-monkey-pwning-a-17-year-old-version-of-spidermonkey/
manugarg
reviewed
Mar 12, 2025
manugarg
reviewed
Mar 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
pacparser currently vendors SpiderMonkey 1.7, a JavaScript engine that predates the Obama presidency. There's been a ton of changes to JavaScript and best practices when it comes to security and portability, so using this old version of SM doesn't make sense anymore.
People are trivially able to write exploits against this old version, and seeing as PAC files could come from untrusted networks, that doesn't seem like a wise decision.
To replace it, I've used duktape, a popular compact and embeddable JS runtime. There are a lot, but duktape seems popular; for example, polkit switched from (newer) SpiderMonkey to duktape. The only change I've needed to make to JS code is that RegExps don't seem to be callable under duktape; they aren't under V8 either though, so this might have been a Mozilla-ism.
The massively smaller codebase of duktape is hopefully better security and maintainability wise, but also results in much smaller binaries. For example, pactester goes from 1.5M to 687K on my system.
Passes unit tests on macOS. Not tested on Linux/Windows yet. However, I'm not certain about i.e. string lifetimes with duktape. They didn't seem clear with SpiderMonkey either though; perhaps it'd be an opportunity to i.e. explicitly strdup them?