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 security issue #364 and non-keyword subgraph parsing #376

Merged
merged 7 commits into from
May 10, 2024

Conversation

sehe
Copy link
Contributor

@sehe sehe commented May 3, 2024

(Note: this PR builds in #375 and #374)

Three commits in this PR:

  1. ad11d2e Fix non-keyword subgraph parsing
  2. 82f623f test_subgraphs verifies (keyword) subgraphs parse
  3. 95b80a9 max_subgraph_nesting_level in read_graphviz_new

The last one actually fixes #364

Works around invalidation of bundle property maps (see boostorg#373).

The `#if SEHE_UNSTABLE_PROPERTY_MAPS_FIXED` section is there to signal
my intent to investigate a generalized fix under that issue. It doubles
as literate documentation of the need for the workaround, so it's less
likely to bite the unwary.
@sehe sehe changed the title Restore broken test_basic_csr_directed_graph Fix security issue #364 and non-keyword subgraph parsing May 3, 2024
@TheZ3ro
Copy link

TheZ3ro commented May 3, 2024

Can confirm this fixes #364 correctly

image

@jeremy-murphy jeremy-murphy self-assigned this May 8, 2024
sehe added 5 commits May 8, 2024 14:39
The code was mostly fine (except for unhygienic `using namespace` in
places), but it was hard to see what was covered.

I've seperated fixtures (sample input + expected output) and this will
simplify invoking the ComparisonDriver (test_graph).
Non-keyword graphs never worked (!). This was uncovered because of
security issue boostorg#364.

parse_subgraph() incorrectly dealt with first_token in the case
where the `subgraph` keyword wasn't used.
@sehe
Copy link
Contributor Author

sehe commented May 8, 2024

Updated for review comments #364 (comment)

Eliminating need for manual re-test after review updates PR boostorg#376
@sehe
Copy link
Contributor Author

sehe commented May 8, 2024

Added test_subgraph_nesting_limit to pin-down the new behavior.

@jeremy-murphy
Copy link
Contributor

Do you need to rebase this? It has all the changes from the second PR. (Never mind the Drone failures, they're just a network glitch.)

@sehe
Copy link
Contributor Author

sehe commented May 9, 2024

Do you need to rebase this? It has all the changes from the second PR. (Never mind the Drone failures, they're just a network glitch.)

Not necessarily. It's how dependent PRs work in Github (note that I started all PRs with the warning and also explicitly stated it before creating them). Therefore, the second PR already had the same dependency, and it worked "fine":

image

Rebasing first might simplify the revision graph every so slightly (it's why I prefer linear-only history with ff-only merges). As a late thought: should I have edited my name into the authors for test/graphviz_test.cpp?

@jeremy-murphy
Copy link
Contributor

OK, I'm still a bit confused as to why it is showing all the changes from the second PR, which is merged, but I'll merge this and see what happens.

@jeremy-murphy jeremy-murphy merged commit 5557ccf into boostorg:develop May 10, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack-overflow in boost::read_graphviz_detail::parser::parse_subgraph
3 participants