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

stack overflow on fragment cycle #716

Closed
qwerdenkerXD opened this issue Nov 1, 2023 · 1 comment · Fixed by #733
Closed

stack overflow on fragment cycle #716

qwerdenkerXD opened this issue Nov 1, 2023 · 1 comment · Fixed by #733
Assignees
Labels
apollo-compiler issues/PRs pertaining to semantic analysis & validation bug Something isn't working

Comments

@qwerdenkerXD
Copy link

qwerdenkerXD commented Nov 1, 2023

Description

During validation aren't all fragment cycles detected.
This causes a stack overflow in latest beta [email protected]

Steps to reproduce

Validate the following query for your schema:

query Introspection{
  __schema {
    types {
      ...cycle
    }
  }
}

fragment cycle on __Type {
  kind
  ...frag
}

fragment frag on __Type {
  kind
  ofType {
    ...cycle
  }
}

Here a code sample.

Expected result

The validation should pass and in the diagnostics should be one like:

{
  "errors": [
    {
      "message": "compiler error: `cycle` fragment cannot reference itself",
      "locations": [
        {
          "line": 9,
          "column": 1
        }
      ]
    }
  ]
}

Actual result

In [email protected]:

thread 'main' has overflowed its stack
fatal runtime error: stack overflow
[1]    2496 abort      ./target/debug/gus start -m testing/server

So the program crashes.

In [email protected]:
An internal error occurs because salsa detected this cycle, so still an error but no crashing stack overflow.
So in terminal it is:

panicked at .../salsa-0.16.1/src/lib.rs:490:48:
Internal error, cycle detected:

DatabaseKeyIndex { group_index: 3, query_index: 43, key_index: 3 }
DatabaseKeyIndex { group_index: 3, query_index: 44, key_index: 3 }
DatabaseKeyIndex { group_index: 3, query_index: 38, key_index: 1 }
DatabaseKeyIndex { group_index: 3, query_index: 43, key_index: 5 }
DatabaseKeyIndex { group_index: 3, query_index: 44, key_index: 5 }
DatabaseKeyIndex { group_index: 3, query_index: 31, key_index: 4 }
DatabaseKeyIndex { group_index: 3, query_index: 43, key_index: 6 }
DatabaseKeyIndex { group_index: 3, query_index: 44, key_index: 6 }
DatabaseKeyIndex { group_index: 3, query_index: 38, key_index: 2 }

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Here the code sample for this version.

Environment

  • Operating system and version: Ubuntu 20.04.6 LTS
  • Shell (bash/zsh/powershell): zsh
  • apollo-rs crate: apollo-compiler
  • Crate version: 1.0.0-beta.5
@qwerdenkerXD qwerdenkerXD added bug Something isn't working triage labels Nov 1, 2023
@qwerdenkerXD qwerdenkerXD changed the title salsa error on fragment cycle stack overflow on fragment cycle Nov 9, 2023
@goto-bus-stop goto-bus-stop added apollo-compiler issues/PRs pertaining to semantic analysis & validation and removed triage labels Nov 9, 2023
@goto-bus-stop
Copy link
Member

goto-bus-stop commented Nov 9, 2023

Thanks for the report, should definitely fix this as it's a potential DOS vector. It's also explicitly outlawed in https://spec.graphql.org/draft/#example-cd11c.

goto-bus-stop added a commit that referenced this issue Nov 9, 2023
Previously fragment cycle detection only counted fragments that are
directly recursive, where the top-level selection for the fragment
included a fragment spread of itself. But spreading a fragment anywhere
inside a nested field is also problematic and prohibited by spec,
because even if the field you're spreading into is nullable, the client
can't know how deep that recursion goes, and it could be infinite.

Fixes #716
@goto-bus-stop goto-bus-stop self-assigned this Nov 9, 2023
goto-bus-stop added a commit that referenced this issue Nov 10, 2023
#733)

* Fix validation stack overflow for fragment cycles inside nested fields

Previously fragment cycle detection only counted fragments that are
directly recursive, where the top-level selection for the fragment
included a fragment spread of itself. But spreading a fragment anywhere
inside a nested field is also problematic and prohibited by spec,
because even if the field you're spreading into is nullable, the client
can't know how deep that recursion goes, and it could be infinite.

Fixes #716

* label every fragment participating in a cycle

* use push() instead of repetitive insert(0,x)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apollo-compiler issues/PRs pertaining to semantic analysis & validation bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants