Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-66g8-4hjf-77xh
Keep a count of the number of open blocks (WIP)
  • Loading branch information
anticomputer committed Mar 31, 2023
2 parents c32ef78 + 6e4493d commit 2300c1b
Show file tree
Hide file tree
Showing 8 changed files with 198 additions and 26 deletions.
1 change: 1 addition & 0 deletions api_test/main.c
Expand Up @@ -1133,6 +1133,7 @@ int main() {
int retval;
test_batch_runner *runner = test_batch_runner_new();

cmark_enable_safety_checks(true);
version(runner);
constructor(runner);
accessors(runner);
Expand Down
6 changes: 6 additions & 0 deletions extensions/table.c
Expand Up @@ -311,12 +311,18 @@ static cmark_node *try_opening_table_header(cmark_syntax_extension *self,
}
}

assert(cmark_node_get_type(parent_container) == CMARK_NODE_PARAGRAPH);
if (!cmark_node_set_type(parent_container, CMARK_NODE_TABLE)) {
free_table_row(parser->mem, header_row);
free_table_row(parser->mem, marker_row);
return parent_container;
}

// Update the node counts after parent_container changed type.
assert(parent_container->next == NULL);
decr_open_block_count(parser, CMARK_NODE_PARAGRAPH);
incr_open_block_count(parser, CMARK_NODE_TABLE);

if (header_row->paragraph_offset) {
try_inserting_table_header_paragraph(parser, parent_container, (unsigned char *)parent_string,
header_row->paragraph_offset);
Expand Down
119 changes: 107 additions & 12 deletions src/blocks.c
Expand Up @@ -70,6 +70,22 @@ static void S_parser_feed(cmark_parser *parser, const unsigned char *buffer,
static void S_process_line(cmark_parser *parser, const unsigned char *buffer,
bufsize_t bytes);

static void subtract_open_block_counts(cmark_parser *parser, cmark_node *node) {
do {
decr_open_block_count(parser, S_type(node));
node->flags &= ~CMARK_NODE__OPEN_BLOCK;
node = node->last_child;
} while (node);
}

static void add_open_block_counts(cmark_parser *parser, cmark_node *node) {
do {
incr_open_block_count(parser, S_type(node));
node->flags |= CMARK_NODE__OPEN_BLOCK;
node = node->last_child;
} while (node);
}

static cmark_node *make_block(cmark_mem *mem, cmark_node_type tag,
int start_line, int start_column) {
cmark_node *e;
Expand Down Expand Up @@ -129,6 +145,7 @@ static void cmark_parser_reset(cmark_parser *parser) {
parser->refmap = cmark_reference_map_new(parser->mem);
parser->root = document;
parser->current = document;
add_open_block_counts(parser, document);

parser->syntax_extensions = saved_exts;
parser->inline_syntax_extensions = saved_inline_exts;
Expand Down Expand Up @@ -242,15 +259,18 @@ static void remove_trailing_blank_lines(cmark_strbuf *ln) {
// Check to see if a node ends with a blank line, descending
// if needed into lists and sublists.
static bool S_ends_with_blank_line(cmark_node *node) {
if (S_last_line_checked(node)) {
return(S_last_line_blank(node));
} else if ((S_type(node) == CMARK_NODE_LIST ||
S_type(node) == CMARK_NODE_ITEM) && node->last_child) {
S_set_last_line_checked(node);
return(S_ends_with_blank_line(node->last_child));
} else {
S_set_last_line_checked(node);
return (S_last_line_blank(node));
while (true) {
if (S_last_line_checked(node)) {
return(S_last_line_blank(node));
} else if ((S_type(node) == CMARK_NODE_LIST ||
S_type(node) == CMARK_NODE_ITEM) && node->last_child) {
S_set_last_line_checked(node);
node = node->last_child;
continue;
} else {
S_set_last_line_checked(node);
return (S_last_line_blank(node));
}
}
}

Expand Down Expand Up @@ -310,6 +330,12 @@ static cmark_node *finalize(cmark_parser *parser, cmark_node *b) {
has_content = resolve_reference_link_definitions(parser, b);
if (!has_content) {
// remove blank node (former reference def)
if (b->flags & CMARK_NODE__OPEN_BLOCK) {
decr_open_block_count(parser, S_type(b));
if (b->prev) {
add_open_block_counts(parser, b->prev);
}
}
cmark_node_free(b);
}
break;
Expand Down Expand Up @@ -382,6 +408,17 @@ static cmark_node *finalize(cmark_parser *parser, cmark_node *b) {
return parent;
}

// Recalculates the number of open blocks. Returns true if it matches what's currently stored
// in parser. (Used to check that the counts in parser, which are updated incrementally, are
// correct.)
bool check_open_block_counts(cmark_parser *parser) {
cmark_parser tmp_parser = {0}; // Only used for its open_block_counts and total_open_blocks fields.
add_open_block_counts(&tmp_parser, parser->root);
return
tmp_parser.total_open_blocks == parser->total_open_blocks &&
memcmp(tmp_parser.open_block_counts, parser->open_block_counts, sizeof(parser->open_block_counts)) == 0;
}

// Add a node as child of another. Return pointer to child.
static cmark_node *add_child(cmark_parser *parser, cmark_node *parent,
cmark_node_type block_type, int start_column) {
Expand All @@ -400,11 +437,14 @@ static cmark_node *add_child(cmark_parser *parser, cmark_node *parent,
if (parent->last_child) {
parent->last_child->next = child;
child->prev = parent->last_child;
subtract_open_block_counts(parser, parent->last_child);
} else {
parent->first_child = child;
child->prev = NULL;
}
parent->last_child = child;
add_open_block_counts(parser, child);

return child;
}

Expand Down Expand Up @@ -1047,8 +1087,14 @@ static cmark_node *check_open_blocks(cmark_parser *parser, cmark_chunk *input,
*all_matched = false;
cmark_node *container = parser->root;
cmark_node_type cont_type;
cmark_parser tmp_parser; // Only used for its open_block_counts and total_open_blocks fields.
memcpy(tmp_parser.open_block_counts, parser->open_block_counts, sizeof(parser->open_block_counts));
tmp_parser.total_open_blocks = parser->total_open_blocks;

assert(check_open_block_counts(parser));

while (S_last_child_is_open(container)) {
decr_open_block_count(&tmp_parser, S_type(container));
container = container->last_child;
cont_type = S_type(container);

Expand All @@ -1060,6 +1106,53 @@ static cmark_node *check_open_blocks(cmark_parser *parser, cmark_chunk *input,
continue;
}

// This block of code is a workaround for the quadratic performance
// issue described here (issue 2):
//
// https://github.com/github/cmark-gfm/security/advisories/GHSA-66g8-4hjf-77xh
//
// If the current line is empty then we might be able to skip directly
// to the end of the list of open blocks. To determine whether this is
// possible, we have been maintaining a count of the number of
// different types of open blocks. The main criterium is that every
// remaining block, except the last element of the list, is a LIST or
// ITEM. The code below checks the conditions, and if they're ok, skips
// forward to parser->current.
if (parser->blank && parser->indent == 0) { // Current line is empty
// Make sure that parser->current doesn't point to a closed block.
if (parser->current->flags & CMARK_NODE__OPEN_BLOCK) {
if (parser->current->flags & CMARK_NODE__OPEN) {
const size_t n_list = read_open_block_count(&tmp_parser, CMARK_NODE_LIST);
const size_t n_item = read_open_block_count(&tmp_parser, CMARK_NODE_ITEM);
// At most one block can be something other than a LIST or ITEM.
if (n_list + n_item + 1 >= tmp_parser.total_open_blocks) {
// Check that parser->current is suitable for jumping to.
switch (S_type(parser->current)) {
case CMARK_NODE_LIST:
case CMARK_NODE_ITEM:
if (n_list + n_item != tmp_parser.total_open_blocks) {
if (parser->current->last_child == NULL) {
// There's another node type somewhere in the middle of
// the list, so don't attempt the optimization.
break;
}
}
// fall through
case CMARK_NODE_CODE_BLOCK:
case CMARK_NODE_PARAGRAPH:
case CMARK_NODE_HTML_BLOCK:
// Jump to parser->current
container = parser->current;
cont_type = S_type(container);
break;
default:
break;
}
}
}
}
}

switch (cont_type) {
case CMARK_NODE_BLOCK_QUOTE:
if (!parse_block_quote_prefix(parser, input))
Expand Down Expand Up @@ -1193,8 +1286,9 @@ static void open_new_blocks(cmark_parser *parser, cmark_node **container,
has_content = resolve_reference_link_definitions(parser, *container);

if (has_content) {

(*container)->type = (uint16_t)CMARK_NODE_HEADING;
cmark_node_set_type(*container, CMARK_NODE_HEADING);
decr_open_block_count(parser, CMARK_NODE_PARAGRAPH);
incr_open_block_count(parser, CMARK_NODE_HEADING);
(*container)->as.heading.level = lev;
(*container)->as.heading.setext = true;
S_advance_offset(parser, input, input->len - 1 - parser->offset, false);
Expand Down Expand Up @@ -1349,7 +1443,7 @@ static void add_text_to_container(cmark_parser *parser, cmark_node *container,
S_set_last_line_blank(container, last_line_blank);

tmp = container;
while (tmp->parent) {
while (tmp->parent && S_last_line_blank(tmp->parent)) {
S_set_last_line_blank(tmp->parent, false);
tmp = tmp->parent;
}
Expand Down Expand Up @@ -1478,6 +1572,7 @@ static void S_process_line(cmark_parser *parser, const unsigned char *buffer,

parser->line_number++;

assert(parser->current->next == NULL);
last_matched_container = check_open_blocks(parser, &input, &all_matched);

if (!last_matched_container)
Expand Down
10 changes: 10 additions & 0 deletions src/cmark-gfm.h
Expand Up @@ -37,6 +37,16 @@ char *cmark_markdown_to_html(const char *text, size_t len, int options);
#define CMARK_NODE_TYPE_MASK (0xc000)
#define CMARK_NODE_VALUE_MASK (0x3fff)

/**
* This is the maximum number of block types (CMARK_NODE_DOCUMENT,
* CMARK_NODE_HEADING, ...). It needs to be bigger than the number of
* hardcoded block types (below) to allow for extensions (see
* cmark_syntax_extension_add_node). But it also determines the size of the
* open_block_counts array in the cmark_parser struct, so we don't want it
* to be excessively large.
*/
#define CMARK_NODE_TYPE_BLOCK_LIMIT 0x20

typedef enum {
/* Error status */
CMARK_NODE_NONE = 0x0000,
Expand Down
30 changes: 20 additions & 10 deletions src/node.c
Expand Up @@ -5,6 +5,16 @@
#include "node.h"
#include "syntax_extension.h"

/**
* Expensive safety checks are off by default, but can be enabled
* by calling cmark_enable_safety_checks().
*/
static bool enable_safety_checks = false;

void cmark_enable_safety_checks(bool enable) {
enable_safety_checks = enable;
}

static void S_node_unlink(cmark_node *node);

#define NODE_MEM(node) cmark_node_mem(node)
Expand Down Expand Up @@ -70,23 +80,23 @@ bool cmark_node_can_contain_type(cmark_node *node, cmark_node_type child_type) {
}

static bool S_can_contain(cmark_node *node, cmark_node *child) {
cmark_node *cur;

if (node == NULL || child == NULL) {
return false;
}
if (NODE_MEM(node) != NODE_MEM(child)) {
return 0;
}

// Verify that child is not an ancestor of node or equal to node.
cur = node;
do {
if (cur == child) {
return false;
}
cur = cur->parent;
} while (cur != NULL);
if (enable_safety_checks) {
// Verify that child is not an ancestor of node or equal to node.
cmark_node *cur = node;
do {
if (cur == child) {
return false;
}
cur = cur->parent;
} while (cur != NULL);
}

return cmark_node_can_contain_type(node, (cmark_node_type) child->type);
}
Expand Down
14 changes: 11 additions & 3 deletions src/node.h
Expand Up @@ -50,12 +50,13 @@ typedef struct {

enum cmark_node__internal_flags {
CMARK_NODE__OPEN = (1 << 0),
CMARK_NODE__LAST_LINE_BLANK = (1 << 1),
CMARK_NODE__LAST_LINE_CHECKED = (1 << 2),
CMARK_NODE__OPEN_BLOCK = (1 << 1),
CMARK_NODE__LAST_LINE_BLANK = (1 << 2),
CMARK_NODE__LAST_LINE_CHECKED = (1 << 3),

// Extensions can register custom flags by calling `cmark_register_node_flag`.
// This is the starting value for the custom flags.
CMARK_NODE__REGISTER_FIRST = (1 << 3),
CMARK_NODE__REGISTER_FIRST = (1 << 4),
};

typedef uint16_t cmark_node_internal_flags;
Expand Down Expand Up @@ -144,6 +145,13 @@ static CMARK_INLINE bool CMARK_NODE_INLINE_P(cmark_node *node) {

CMARK_GFM_EXPORT bool cmark_node_can_contain_type(cmark_node *node, cmark_node_type child_type);

/**
* Enable (or disable) extra safety checks. These extra checks cause
* extra performance overhead (in some cases quadratic), so they are only
* intended to be used during testing.
*/
CMARK_GFM_EXPORT void cmark_enable_safety_checks(bool enable);

#ifdef __cplusplus
}
#endif
Expand Down
39 changes: 39 additions & 0 deletions src/parser.h
Expand Up @@ -50,8 +50,47 @@ struct cmark_parser {
cmark_llist *syntax_extensions;
cmark_llist *inline_syntax_extensions;
cmark_ispunct_func backslash_ispunct;

/**
* The "open" blocks are the blocks visited by the loop in
* check_open_blocks (blocks.c). I.e. the blocks in this list:
*
* parser->root->last_child->...->last_child
*
* open_block_counts is used to keep track of how many of each type of
* node are currently in the open blocks list. Knowing these counts can
* sometimes help to end the loop in check_open_blocks early, improving
* efficiency.
*
* The count is stored at this offset: type - CMARK_NODE_TYPE_BLOCK - 1
* For example, CMARK_NODE_LIST (0x8003) is stored at offset 2.
*/
size_t open_block_counts[CMARK_NODE_TYPE_BLOCK_LIMIT];
size_t total_open_blocks;
};

static CMARK_INLINE void incr_open_block_count(cmark_parser *parser, cmark_node_type type) {
assert(type > CMARK_NODE_TYPE_BLOCK);
assert(type <= CMARK_NODE_TYPE_BLOCK + CMARK_NODE_TYPE_BLOCK_LIMIT);
parser->open_block_counts[type - CMARK_NODE_TYPE_BLOCK - 1]++;
parser->total_open_blocks++;
}

static CMARK_INLINE void decr_open_block_count(cmark_parser *parser, cmark_node_type type) {
assert(type > CMARK_NODE_TYPE_BLOCK);
assert(type <= CMARK_NODE_TYPE_BLOCK + CMARK_NODE_TYPE_BLOCK_LIMIT);
assert(parser->open_block_counts[type - CMARK_NODE_TYPE_BLOCK - 1] > 0);
parser->open_block_counts[type - CMARK_NODE_TYPE_BLOCK - 1]--;
assert(parser->total_open_blocks > 0);
parser->total_open_blocks--;
}

static CMARK_INLINE size_t read_open_block_count(cmark_parser *parser, cmark_node_type type) {
assert(type > CMARK_NODE_TYPE_BLOCK);
assert(type <= CMARK_NODE_TYPE_BLOCK + CMARK_NODE_TYPE_BLOCK_LIMIT);
return parser->open_block_counts[type - CMARK_NODE_TYPE_BLOCK - 1];
}

#ifdef __cplusplus
}
#endif
Expand Down
5 changes: 4 additions & 1 deletion src/syntax_extension.c
Expand Up @@ -29,7 +29,10 @@ cmark_syntax_extension *cmark_syntax_extension_new(const char *name) {
cmark_node_type cmark_syntax_extension_add_node(int is_inline) {
cmark_node_type *ref = !is_inline ? &CMARK_NODE_LAST_BLOCK : &CMARK_NODE_LAST_INLINE;

if ((*ref & CMARK_NODE_VALUE_MASK) == CMARK_NODE_VALUE_MASK) {
if ((*ref & CMARK_NODE_VALUE_MASK) >= CMARK_NODE_TYPE_BLOCK_LIMIT) {
// This assertion will fail if you try to register more extensions than
// are currently allowed by CMARK_NODE_TYPE_BLOCK_MAXNUM. Try increasing
// the limit.
assert(false);
return (cmark_node_type) 0;
}
Expand Down

0 comments on commit 2300c1b

Please sign in to comment.