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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't rely on strict mode behaviour for arguments #3470
Conversation
Can you describe the different behaviour by another simple Node.js script? Something like,
|
I have more information on this. It turns out that the problem is in conjunction with pino-debug, which decorates Node's module wrapper function, adding statements before the module's code is executed. That way, the So, the issue is not with axios, but rather with pino-debug, since it invalidates all This PR can be rejected if you want, but I would still argue that relying on strict mode in this way is bad form. It's so easy to not use I will leave it up to you whether to merge this or not. Thanks 馃憢 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strict mode code doesn't alias properties of arguments objects created within it
I see.
Currently, the main
axios.request
method/axios
function relies on JavaScript's Strict Mode's behaviour ofarguments
. Here's the relevant MDN sectionThis should be fine, since the file is, in fact, in strict mode.
However, we've discovered that for some reason in our application this behaviour isn't maintained by Node v14.15.1.
In the screenshot above, you can see how
config
being reassigned to (effectively){}
changes the value ofarguments[0]
, which is exactly the behaviour that strict mode changes.Subsequently, the
config.url = arguments[0];
statement effectively doesconfig.url = config;
making a circular data structure, leading to stack overflows down the line.I don't know why this is happening, and I can't make a test since the test environment correctly follows the strict mode behaviour. But, of course, if we don't rely on this particular behaviour, the problem goes away altogether.
Please consider merging this PR since this bug effectively injected a stack overflow bomb into our production environment, and I wouldn't want to see that happen to anyone else 馃槃