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

[Fix] stringify: actually fix cyclic references #426

Merged
merged 1 commit into from Dec 6, 2021

Conversation

liaokunhua
Copy link
Contributor

@liaokunhua liaokunhua commented Dec 4, 2021

With all the testing, I think this is the ultimate solution for circular references.

Fixes #403.

Fixes ljharb#403.

Co-authored-by: liaokunhua <acutedev@163.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide a regression test that this fixes?

lib/stringify.js Outdated Show resolved Hide resolved
lib/stringify.js Outdated Show resolved Hide resolved
@liaokunhua
Copy link
Contributor Author

Can you provide a regression test that this fixes?

Sorry bro, this is a bit out of my league, I haven't thought of a special object that would prove i didn't fix this bug.

So, let's talk about how I found this bug.
I started learning front-end and github a few days ago, starting with a simple project.
I noticed that the stringify.js file in the test report had a line of code that was not covered.

=> throw new RangeError('Cyclic object value');

I did some tests, the result is that non-circular duplicated references can work, but circular dependencies result in "RangeError: Maximum call stack size exceeded", not "RangeError: Cyclic object value".

I think the exception thrown in the use case test of the circular dependency happens to be RangeError, but the exception message is not checked.

@ljharb
Copy link
Owner

ljharb commented Dec 5, 2021

Surely you provided some input that caused that error?

lib/stringify.js Outdated Show resolved Hide resolved
@liaokunhua
Copy link
Contributor Author

Surely you provided some input that caused that error?

Right, an example of a circular reference is constructed, which is inside the use case test.
It is easy to reproduce.

Based on the original code for testing.
Step1:
modify line 79 in the stringify.js file, throw new RangeError('Cyclic object value'); to throw new Error('Cyclic object value');.

Step2:
Create a new test and select an example of a circular reference in the stringify.js file inside the test folder and make the changes.

t.test("does not crash when parsing circular references", function(st) {
        var circular = { a: 'value' };
        circular.a = circular;
        try {
            qs.stringify(circular);
        } catch(e) {
            console.dir(e)
        }
        st['throws'](
            function () { qs.stringify(circular); },
            RangeError,
            'Cyclic values throw'
        );
        st.end();
})

Step3:
Run test and get output:

TAP version 13
# stringify()
# does not crash when parsing circular references
RangeError: Maximum call stack size exceeded
// ....omitted stack info
ok 1 Cyclic values throw

1..1
# tests 1
# pass  1

# ok

I was dumbfounded!! not the expected "Error:Cyclic object value".

Environment information:
system: ubuntu 20.04
node: v14.16.1
npm: 6.14.12

@ljharb
Copy link
Owner

ljharb commented Dec 5, 2021

ah, i see - so changing that test to check the error message - so we know it's not a stack overflow - would reproduce the error that this PR fixes?

if so, could you make that change?

@liaokunhua
Copy link
Contributor Author

ah, i see - so changing that test to check the error message - so we know it's not a stack overflow - would reproduce the error that this PR fixes?

if so, could you make that change?

emmm...kinda wish it was me, but i don't think i have it in me yet.😅

@ljharb ljharb changed the title stringify: fix cyclic reference [Fix] stringify: actually fix cyclic references Dec 6, 2021
@ljharb
Copy link
Owner

ljharb commented Dec 6, 2021

I rebased this PR, and updated the test cases in the default branch that were asserting a RangeError, but not the right one, and added another test case that confirms a fix for #403.

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #426 (9aee773) into master (24c19cc) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #426      +/-   ##
==========================================
+ Coverage   99.78%   99.85%   +0.07%     
==========================================
  Files           8        8              
  Lines        1393     1408      +15     
  Branches      169      172       +3     
==========================================
+ Hits         1390     1406      +16     
+ Misses          3        2       -1     
Impacted Files Coverage Δ
lib/stringify.js 100.00% <100.00%> (+0.70%) ⬆️
test/stringify.js 99.75% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24c19cc...9aee773. Read the comment docs.

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually i'll merge this as-is, to fix the bug; but i'd love it if there was a followup to clean up the loop.

@ljharb ljharb merged commit 9aee773 into ljharb:master Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

qs.stringify Uncaught RangeError: Cyclic object value
2 participants