-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Signed integer overflow in latest node 18 LTS release #48621
Comments
The patch is a relatively simple find&replace but I wasn't sure if that's an acceptable patch in |
This will likely be fixed in #48345 but the fact that we ended up with undefined behavior (which yes, didn't appear to break anything, maybe) in an LTS release make me question the recent pace of adding new C++ deps. It's a lot of new, non-trivial, and non-hardened C++ code entering core. |
Thanks for the report. I'm one of the authors of Ada. As you said, this is fixed in Ada v2.x, and is pending to be merged into the v18-x staging branch. (cc @nodejs/releasers)
Any changes to deps are prohibited and it's recommended for contributors to upstream any issues & fixes.
Unfortunately, Ada was the default URL parser for Node 19 and Node 20, and because most people prefer to stay in LTS, it is impossible to have the same amount of user feedback as a package would receive from a Node.js LTS release. Practically speaking, I believe it's inevitable to catch bugs like these without a certain users trying/using it.
Here's the timeframe:
To summarize: it took 74 days to land a new dependency to LTS release.
I disagree with your non-trivial classification. URL is one of the most important and used components in the Node.js runtime. If you have any specific recommendations about how to classify Ada as a hardened dependency, please open an issue to Ada's repository, and I'll be happy personally to follow up with them. Regarding the status of Ada, let me give you some insight: Ada has more tests than Web platform tests (which influenced URL parsers like curl and boost/url with finding bugs in their implementation, and has a significant test coverage & fuzzing coverage in Google's OSS Fuzz. But sometimes, it is not sufficient to catch bugs like these due to specific constraints, and despite how much code you write, and how much test you have in your project from time to time some significant bugs like these pass through. cc @nodejs/url |
This PR should help too |
I think that @RafaelGSS’s answer is the right one. I expect that the community does not want to discourage fast paced contributions that improve the performance of Node. However, they also want high reliability. The two values are not in opposition: you can get more quality by improving testing as part of CI, improving tooling etc. In turn, this can make you more confident and able to receive contributions. Regarding the URL parsing…
Generally, my impression is Node should ramp up both the testing and the benchmarking… It seems that’s what Rafael is doing. |
Thanks for the detailed response and for your work making URL parsing faster in node!
I'm generally aware of that but I wasn't sure how that policy deals with cases like this where node is using a major version that seemingly is no longer maintained upstream. If this was a more critical issue - would fixing it be blocked on doing a major version upgrade of the dependency? Or would we hope that upstream would create a new release line with a backport of the fix?
By "non-trivial" I meant "not just a few lines of obviously-correct code". I didn't meant to imply anything about how important or heavily used the code is. We can definitely disagree on whether 2.5k LOC is "trivial" or not. I personally couldn't skim it and say "yep, that's all correct" which is roughly the bar I'd set for "trivial code". I'd be surprised if many developers could and that is the important part to me here, not the term "trivial".
I definitely can understand that there's a tricky chicken&egg thing going on here. But in practice, this will likely mean that we're stuck on an older 18.x and can't upgrade to the latest version and the security fixes that brings. Which means that we get neither the faster URL parsing (which would have been a neat but non-critical win) nor all the other fixes (which we'd actually care about). So ada is getting real world exposure but at the expense of at least one (group of a few thousand) real world users not being able to upgrade within an LTS line. I'll readily admit that our little world is "special" since a typical nodejs setup doesn't require that ~all test suites pass with a bunch of sanitizers enabled. But I also can't tell teams to disable sanitizers on CI for a bit because node is incompatible.
To me, by definition, hardening only happens over time and with real usage. Which is a frustrating answer because it's not "actionable" but that cost of new code is very hard to "solve". New code will always be unstable (see also the very quick major bump). But I want to be a bit more clear here: When I pointed out the pace of new native deps, I wasn't talking about ada in particular. Of all the new deps, ada is likely the one I have the least concerns about. Within the lifetime of node 18, multiple new native deps were added:
The issue in the OP is the one that I could easily identify and already have a workaround for. There's a second one in ada that I still can't easily reproduce that causes a failed assertion in the Which is totally fine, btw! This isn't really blocking anything and because of the way we're running nodejs, we can safely skip 18.16.1. The only fallout is a couple of days of wasted time on my end which I'll survive. ;) |
You ran Node with ubsan, but ubsan is not used as part of continuous integration tests of Node. In fact, it appears that it is not used because Node is not generally ubsan clean, at least as far as the comments on this issue are concerned: #46297
And these problems predate the recent changes you allude to. It is perfectly reasonable to demand the Node be ubsan clean, and I think it is reasonable to expect new dependencies to be ubsan clean... but it seems that it is not a goal that has been reached yet, generally. |
@lemire Our nodejs binaries are special™ in many ways. They link against BoringSSL, have dedicated V8 imports, are built using bazel, etc.. So I don't have any expectations of them being officially supported. The good news is that they've been ubsan clean for many years now, so code within nodejs core itself is usually already clean and I'm always happy to upstream fixes if there's any sanitizer errors. The reason I opened this issue and made the comment above is that these kinds of failures are actually very rare and hadn't happened in multiple years. So node used to be fairly asan/ubsan clean but has stopped being reliably clean in the more recent past. And the majority (admittedly, small sample size) of issues came from the new deps with only one coming from node core itself (#48566). |
Version
18.16.1
Platform
No response
Subsystem
deps/ada
What steps will reproduce the bug?
As far as I can tell, any use of the URL constructor which now calls into native code from ada:
This is reported when building with
ubsan
.How often does it reproduce? Is there a required condition?
No response
What is the expected behavior? Why is that the expected behavior?
No undefined behavior caused by using the URL constructor.
What do you see instead?
An integer overflow.
Additional information
This was fixed upstream by using uint64 literals instead of signed ints: ada-url/ada@38d6eae.
The text was updated successfully, but these errors were encountered: