From 5007b01ad547bf0413280ea381e4a18210dbae5c Mon Sep 17 00:00:00 2001 From: sehe Date: Thu, 2 May 2024 05:22:04 +0200 Subject: [PATCH 1/4] Fix non-keyword subgraph parsing Non-keyword graphs never worked (!). This was uncovered because of security issue #364. parse_subgraph() incorrectly dealt with first_token in the case where the `subgraph` keyword wasn't used. --- src/read_graphviz_new.cpp | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/read_graphviz_new.cpp b/src/read_graphviz_new.cpp index fdc6e95f4..d97292b41 100644 --- a/src/read_graphviz_new.cpp +++ b/src/read_graphviz_new.cpp @@ -207,7 +207,7 @@ namespace read_graphviz_detail tokenizer(const std::string& str) : begin(str.begin()), end(str.end()) { - std::string end_of_token = "(?=(?:\\W))"; + // std::string end_of_token = "(?=(?:\\W))"; // SEHE: unused? std::string whitespace = "(?:\\s+)"; std::string slash_slash_comment = "(?://.*?$)"; std::string slash_star_comment = "(?:/\\*.*?\\*/)"; @@ -773,10 +773,18 @@ namespace read_graphviz_detail bool is_anonymous = true; if (first_token.type == token::kw_subgraph) { - if (peek().type == token::identifier) + switch (peek().type) { + case token::identifier: name = get().normalized_value; is_anonymous = false; + break; + case token::left_brace: + is_anonymous = true; + break; + default: + error("Subgraph reference needs a name"); + break; } } if (is_anonymous) @@ -790,19 +798,19 @@ namespace read_graphviz_detail = current(); // Initialize properties and defaults subgraphs[name].members.clear(); // Except member list } - if (first_token.type == token::kw_subgraph - && peek().type != token::left_brace) + if (!is_anonymous && peek().type != token::left_brace) { - if (is_anonymous) - error("Subgraph reference needs a name"); return name; } subgraph_name old_sg = current_subgraph_name; current_subgraph_name = name; - if (peek().type == token::left_brace) - get(); - else - error("Wanted left brace to start subgraph"); + if (first_token.type != token::left_brace) + { + if (peek().type == token::left_brace) + get(); + else + error("Wanted left brace to start subgraph"); + } parse_stmt_list(); if (peek().type == token::right_brace) get(); From 9c0ceda4c86334c87f972d917a8c10ee1c8f6d33 Mon Sep 17 00:00:00 2001 From: sehe Date: Thu, 2 May 2024 05:21:27 +0200 Subject: [PATCH 2/4] test_subgraphs verifies (keyword) subgraphs parse --- test/graphviz_test.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/graphviz_test.cpp b/test/graphviz_test.cpp index 400732176..a9339a99c 100644 --- a/test/graphviz_test.cpp +++ b/test/graphviz_test.cpp @@ -440,6 +440,27 @@ void test_basic_csr_directed_graph_ext_props() edge_weight); } +void test_subgraphs() { + // on the BGL side, the new parser doesn't support subgraphs + // however, the docs promise to support reading them on the input side as "syntactic sugar". + for (auto gv : { + Fixture { "digraph {}" }, + Fixture { "digraph { 1 -> {} }", 1 }, + Fixture { "digraph { 1 -> {2} }", 2 }, + Fixture { "digraph { 1; { 2; 3; } }", 3 }, + Fixture { "digraph { { 2; 3; } 1; }", 3 }, + Fixture { "digraph { 1; subgraph { 2; 3; } }", 3 }, + Fixture { "digraph { 1 -> subgraph { 2; 3; } }", 3 }, + Fixture { "digraph { 1 -> subgraph hello { 2; 3; } }", 3 }, + Fixture { "digraph { 1 -> subgraph clust_Hello { 2; 3; } }", 3 }, + Fixture { "digraph { 1 -> subgraph \"hello\" { 2; 3; } }", 3 }, + Fixture { "digraph { {2} -> subgraph \"hello\" {{{{{{{{{{{{{{{{{{{{{{{{ 2; 3; }}}}}}}}}}}}}}}}}}}}}}}} }", 2 }, + }) + { + TEST_GRAPH(Models::DiGraph, gv); + } +} + int main() { test_basic_directed_graph_1(); @@ -457,5 +478,6 @@ int main() test_comments_embedded_in_strings(); test_basic_csr_directed_graph_ext_props(); test_basic_csr_directed_graph(); + test_subgraphs(); return boost::report_errors(); } From c71ceb8486a53e3fff6d5cdbcfba72d740ec1671 Mon Sep 17 00:00:00 2001 From: sehe Date: Thu, 2 May 2024 06:00:11 +0200 Subject: [PATCH 3/4] max_subgraph_nesting_level in read_graphviz_new --- src/read_graphviz_new.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/read_graphviz_new.cpp b/src/read_graphviz_new.cpp index d97292b41..99fc81bc4 100644 --- a/src/read_graphviz_new.cpp +++ b/src/read_graphviz_new.cpp @@ -53,6 +53,7 @@ namespace boost namespace read_graphviz_detail { + static const long max_subgraph_nesting_level = 255; struct token { enum token_type @@ -527,6 +528,7 @@ namespace read_graphviz_detail std::map< subgraph_name, subgraph_info > subgraphs; std::string current_subgraph_name; int sgcounter; // Counter for anonymous subgraphs + long sgnesting_level; std::set< std::pair< node_name, node_name > > existing_edges; // Used for checking in strict graphs @@ -538,7 +540,7 @@ namespace read_graphviz_detail subgraph_member_list& current_members() { return current().members; } parser(const std::string& gr, parser_result& result) - : the_tokenizer(gr), lookahead(), r(result), sgcounter(0) + : the_tokenizer(gr), lookahead(), r(result), sgcounter(0), sgnesting_level(0) { current_subgraph_name = "___root___"; current() = subgraph_info(); // Initialize root graph @@ -803,6 +805,10 @@ namespace read_graphviz_detail return name; } subgraph_name old_sg = current_subgraph_name; + if (++sgnesting_level > max_subgraph_nesting_level) + { + error("Exceeded maximum subgraph nesting level"); + } current_subgraph_name = name; if (first_token.type != token::left_brace) { @@ -817,6 +823,7 @@ namespace read_graphviz_detail else error("Wanted right brace to end subgraph"); current_subgraph_name = old_sg; + sgnesting_level -= 1; return name; } From 05aa9fd8169597064ac86a64f48c05cc638e6851 Mon Sep 17 00:00:00 2001 From: sehe Date: Wed, 8 May 2024 15:35:10 +0200 Subject: [PATCH 4/4] Include test for subgraph nesting limit Eliminating need for manual re-test after review updates PR #376 --- test/graphviz_test.cpp | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/test/graphviz_test.cpp b/test/graphviz_test.cpp index a9339a99c..c29b05f70 100644 --- a/test/graphviz_test.cpp +++ b/test/graphviz_test.cpp @@ -440,9 +440,11 @@ void test_basic_csr_directed_graph_ext_props() edge_weight); } -void test_subgraphs() { +void test_subgraphs() +{ // on the BGL side, the new parser doesn't support subgraphs - // however, the docs promise to support reading them on the input side as "syntactic sugar". + // however, the docs promise to support reading them on the input side as + // "syntactic sugar". for (auto gv : { Fixture { "digraph {}" }, Fixture { "digraph { 1 -> {} }", 1 }, @@ -454,13 +456,33 @@ void test_subgraphs() { Fixture { "digraph { 1 -> subgraph hello { 2; 3; } }", 3 }, Fixture { "digraph { 1 -> subgraph clust_Hello { 2; 3; } }", 3 }, Fixture { "digraph { 1 -> subgraph \"hello\" { 2; 3; } }", 3 }, - Fixture { "digraph { {2} -> subgraph \"hello\" {{{{{{{{{{{{{{{{{{{{{{{{ 2; 3; }}}}}}}}}}}}}}}}}}}}}}}} }", 2 }, + Fixture { + "digraph { {2} -> subgraph \"hello\" {{{{ 2; 3; }}}} }", 2 }, }) { TEST_GRAPH(Models::DiGraph, gv); } } +void test_subgraph_nesting_limit() // issue #364 +{ + auto independent_nests = [=](unsigned level) + { + auto sg = std::string(level, '{') + " 2; 3; " + std::string(level, '}'); + ComparisonDriver::test_graph< Models::DiGraph >( + { "digraph{1->" + sg + "}", 3 }); + ComparisonDriver::test_graph< Models::DiGraph >( + { "digraph{1->" + sg + ";4->" + sg + "}", 4 }); + }; + + constexpr unsigned limit = 255; + independent_nests(1); + independent_nests(limit / 2); + independent_nests(limit - 1); + independent_nests(limit); // edge-case + BOOST_TEST_THROWS(independent_nests(limit + 1), boost::bad_graphviz_syntax); +} + int main() { test_basic_directed_graph_1(); @@ -479,5 +501,6 @@ int main() test_basic_csr_directed_graph_ext_props(); test_basic_csr_directed_graph(); test_subgraphs(); + test_subgraph_nesting_limit(); return boost::report_errors(); }