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

Handle closing flow sequence after explicit key #295

Merged
merged 1 commit into from
May 7, 2024

Conversation

perlpunk
Copy link
Member

@perlpunk perlpunk commented May 6, 2024

Currently after an explicit flow key '?' in a flow sequence, an immediately following closing ] is ignored by the parser:

% echo '[ ? ]' | ./tests/run-parser-test-suite --flow keep
+STR
+DOC
+SEQ []
+MAP {}
=VAL :
=VAL :
-MAP
Parse error: did not find expected ',' or ']'
Line: 2 Column: 1
% echo '[ ? ] ]' | ./tests/run-parser-test-suite --flow keep
+STR
+DOC
+SEQ []
+MAP {}
=VAL :
=VAL :
-MAP
-SEQ
-DOC
-STR

It is read correctly by the scanner as a YAML_FLOW_SEQUENCE_END_TOKEN, and the flow_level is decreased. Then it is passed to the parser where it gets ignored.
This leads to invalid YAML being accepted, and valid YAML resulting in an error.

Also the flow_level is incorrectly decreased, so you can nest sequences like that without running in to the MAX_NESTING_LEVEL.

Currently after an explicit flow key '?' in a flow sequence, an immediately
following closing ] is ignored by the parser:

    % echo '[ ? ]' | ./tests/run-parser-test-suite --flow keep
    +STR
    +DOC
    +SEQ []
    +MAP {}
    =VAL :
    =VAL :
    -MAP
    Parse error: did not find expected ',' or ']'
    Line: 2 Column: 1
    % echo '[ ? ] ]' | ./tests/run-parser-test-suite --flow keep
    +STR
    +DOC
    +SEQ []
    +MAP {}
    =VAL :
    =VAL :
    -MAP
    -SEQ
    -DOC
    -STR

It is read correctly by the scanner as a YAML_FLOW_SEQUENCE_END_TOKEN, and
the flow_level is decreased. Then it is passed to the parser where it gets
ignored.
This leads to invalid YAML being accepted, and valid YAML resulting in an
error.

Also the flow_level is incorrectly decreased, so you can nest sequences
like that without running in to the MAX_NESTING_LEVEL.
Copy link
Member

@ingydotnet ingydotnet left a comment

Choose a reason for hiding this comment

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

[{?}] and [{?}]] are similar. Can you verify it fixes those?

Tests should probably be added.

@perlpunk
Copy link
Member Author

perlpunk commented May 6, 2024

[{?}] and [{?}]] are similar. Can you verify it fixes those?

Those are not affected by the code and work like expected already.

Tests should probably be added.

I will add tests to the yaml-test-suite.

@ingydotnet
Copy link
Member

Oh you are right. Misread again.
Approved.

@perlpunk perlpunk merged commit 588eabf into yaml:master May 7, 2024
4 checks passed
@perlpunk perlpunk deleted the fix-explicit-flow branch May 7, 2024 14:06
perlpunk added a commit to perlpunk/libyaml that referenced this pull request May 20, 2024
The fix in yaml#295 was not correct.

    # cat a.yaml
    [?]

    # Before
    % ./tests/run-parser-test-suite --flow keep < a.yaml
    +STR
    +DOC ---
    +SEQ []
    +MAP {}
    -SEQ
    -DOC
    -STR

    % ./tests/run-loader a.yaml
    [1] Loading 'a.yaml': run-loader: loader.c:470: yaml_parser_load_sequence_end: Assertion `parser->document->nodes.start[index-1].type == YAML_SEQUENCE_NODE' failed.
    [1]    21446 IOT instruction (core dumped)  ./tests/run-loader a.yaml

    # After
    % ./tests/run-parser-test-suite --flow keep < a.yaml
    +STR
    +DOC ---
    +SEQ []
    +MAP {}
    =VAL :
    =VAL :
    -MAP
    -SEQ
    -DOC
    -STR

    % ./tests/run-loader a.yaml
    [1] Loading 'a.yaml': SUCCESS (1 documents)
perlpunk added a commit to perlpunk/libyaml that referenced this pull request May 20, 2024
The fix in yaml#295 was not correct.

    # cat a.yaml
    ---
    [?]

    # Before
    % ./tests/run-parser-test-suite --flow keep < a.yaml
    +STR
    +DOC ---
    +SEQ []
    +MAP {}
    -SEQ
    -DOC
    -STR

    % ./tests/run-loader a.yaml
    [1] Loading 'a.yaml': run-loader: loader.c:470: yaml_parser_load_sequence_end: Assertion `parser->document->nodes.start[index-1].type == YAML_SEQUENCE_NODE' failed.
    [1]    21446 IOT instruction (core dumped)  ./tests/run-loader a.yaml

    # After
    % ./tests/run-parser-test-suite --flow keep < a.yaml
    +STR
    +DOC ---
    +SEQ []
    +MAP {}
    =VAL :
    =VAL :
    -MAP
    -SEQ
    -DOC
    -STR

    % ./tests/run-loader a.yaml
    [1] Loading 'a.yaml': SUCCESS (1 documents)
perlpunk added a commit to perlpunk/libyaml that referenced this pull request May 20, 2024
The fix in yaml#295 was not correct.

    # cat a.yaml
    ---
    [?]

    # Before
    % ./tests/run-parser-test-suite --flow keep < a.yaml
    +STR
    +DOC ---
    +SEQ []
    +MAP {}
    -SEQ
    -DOC
    -STR

    % ./tests/run-loader a.yaml
    [1] Loading 'a.yaml': run-loader: loader.c:470: yaml_parser_load_sequence_end: Assertion `parser->document->nodes.start[index-1].type == YAML_SEQUENCE_NODE' failed.
    [1]    21446 IOT instruction (core dumped)  ./tests/run-loader a.yaml

    # After
    % ./tests/run-parser-test-suite --flow keep < a.yaml
    +STR
    +DOC ---
    +SEQ []
    +MAP {}
    =VAL :
    =VAL :
    -MAP
    -SEQ
    -DOC
    -STR

    % ./tests/run-loader a.yaml
    [1] Loading 'a.yaml': SUCCESS (1 documents)
@perlpunk
Copy link
Member Author

This PR caused a problem, and is now fixed with #296

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.

2 participants