-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Node crashes on new URL #48254
Comments
Is this somehow platform-specific? I can't reproduce on Windows (I tried with all most recent node versions, and v19.8.1). |
Ok, just tried v19.8.1 on Debian 5.10.158-2 (2022-12-13) x86_64 GNU/Linux, cannot reproduce it there either. The relevant difference is that in Debian, I'm seeing /usr/include/c++/10/string_view, whereas in Arch Linux, it's version 12.2.1. Perhaps that's why. |
cc @nodejs/url @lemire |
I can't reproduce on my arch linux machine. Also tried running under
|
Perhaps you could post a backtrace from gdb or valgrind output? |
I think we're hitting the assert added in gcc-mirror/gcc@3eefb30 ? I'm not comfortable with C++ but I guess it uncovers a bug somewhere in Node.js or Ada code ? |
I'm not sure how to do the gdb debugging? I tried the inspect, but not sure how helpful that could still be:
I reviewed the code, if the relevant code is indeed in
The error must occur at line 215 of
EDIT: Missed something. |
@jetibest can you try it with the last node.js version? |
Create a script which contains the offending code (e.g.,
Note that you can use lldb also. |
Once it crashes, you can type |
I have created a test that should replicate this bug in ada at ada-url/ada#421 So far none of the CI runs have hit a problem. |
It does not look like we can reproduce the problem in ada per se. |
If the problem is in ada, my guess would be that we could fix it by upgrading ada in node 19. |
Node.js 19 is no longer supported (except for critical or security fixes) |
@targos If a certain user input can potentially lead to crashing Node, which cannot be caught as an exception as expected, then this could technically be exploited to crash the application (= DoS attack). It doesn't really matter to me personally, but wouldn't this classify it as a security bug? @lemire Thank you for the mini-tutorial, see the results below: I made a script at
Then I ran
|
It is not an exception. It seems that Node.js is compiled without NDEBUG which means that asserts translate into a terminate. In this sense, the crash is deliberate. That is, Node is not meant to recover from the assert. |
Just upgraded NodeJS to v20.2.0, unable to reproduce this issue there. |
It is possible that earlier versions of ada had a bug that is being triggered under some conditions. If we think that there is an issue, the solution would be to update ada in Node.js 19 and issue a patch release. Node 19.8.1 uses ada 1.0.4. We are now at ada 2.5.0. I should say that I could not reproduce the issue.
Yes. It looks to me that the commit you refer to goes back to 2020 and is part of the releases 11.3 and later, 12.1 and later. We test ada with these compilers. |
If it's not reproducible / is fixed in v20.2.0, I suggest we close this issue. Node.js 19 goes EoL tomorrow. |
Version
v19.8.1
Platform
Linux pc 6.2.7-arch1-1 #1 SMP PREEMPT_DYNAMIC Sat, 18 Mar 2023 01:06:36 +0000 x86_64 GNU/Linux
Subsystem
url
What steps will reproduce the bug?
Running in Node:
How often does it reproduce? Is there a required condition?
There's no issue when the second argument is only a hostname, or a port. It only appears to lead to an issue when both the hostname and port is given in the second argument.
What is the expected behavior? Why is that the expected behavior?
An Error should be thrown with code
ERR_INVALID_URL
:What do you see instead?
The error given in the steps to reproduce shows up, and Node crashes.
Additional information
No response
The text was updated successfully, but these errors were encountered: