Crash with non printable characters injected to YAML file during FUZZ testing...

[winneymj]

Hi,
I have a YAML file that contains non printable ASCII characters that were added to the file during Fuzz testing to harden the product.
Problem is that the Parser seems to ignore these characters and does not generate an error in the callback.
(We are on an older version 0.1.1.8)
Later in the code when a Node.val() is accessed it can throw exception.
Below is text (you do not see the non printable chars, unless viewed by Hex editor)

Project:
 Name: test_project_file_7ed30cb41d
 Nodes:
 - 󠁓File: Nodes/test_project_file_7ed30cb41d.yaml

Attached is the file: test_project_file_7ed30cb41d.txt

The offending chars are: f3 a0 81 93 (after the Nodes: )

My question is does the parser detect non printable characters like these?
Perhaps newer version can handle and error on these?
I think we may build from source and produce our own library and do not have access to that system, so do not want to go through all that upgrade unless I know it will be able to handle this.
Any help is appreciated

Thanks
Mark

EDIT : here is the MVE:

#include <iostream>
#include <iomanip>
#include <cstdlib>

#include "ryml_std.hpp"
#include "c4/yml/tree.hpp"
#include "c4/yml/parse.hpp"

using namespace c4::yml;

void ErrorCallback(char const* message, size_t, Location, void*)
{
	std::cout << "ErrorCallback:" << message << std::endl;
}

std::string GetStringFrom(c4::csubstr const& nodeValue)
{
	std::string result;
	from_chars(nodeValue, &result);

	return result;
}

int main(int argc, char* argv[])
{
	Callbacks callbacks(nullptr, nullptr, nullptr, ErrorCallback);
	set_callbacks(callbacks);

	const char yml_buf[] = u8"Project:\n Name: test_project_file_7ed30cb41d\n Nodes:\n - xF3xA0x81x93󠁓File: Nodes/test_project_file_7ed30cb41d.yaml";

	Tree tree = parse(yml_buf);
	if (tree.empty())
		std::cout << "parse failed" << std::endl;

	NodeRef projectNode = tree["Project"];
	NodeRef nodes = projectNode["Nodes"];
	for (auto const& node : nodes.children())
	{
		auto const fileNode = node["File"]; // Error callback is called when in debug. but not in release
		auto const value = fileNode.val();
		std::cout << "value=" << value <<std::endl;
		GetStringFrom(value);
	}
}

[biojppm]

EDIT: @winneymj Now that an MVE is present, the problem is clear. The crash occurs because you're trying to get the value on a non-existing node. It is up to you to check whether the node exists, but Debug builds help you detect the problem.

The special characters come not after Nodes: (as originally described) but on the seq opened after it, ie after Nodes:\n - . So the - File: that you assume to be there is actually - xF3xA0x81x93󠁓File:, and thus node has no member with key File; it is xF3xA0x81x93󠁓File instead. That's why you cannot get node["File"].val(): node["File"] does not exist unless you create it (is a seed node, it is not valid, and only if you write to it via = or << will it gain a val). That's also what the assertion is telling you.

In short, if you use node["xF3xA0x81x93󠁓File"].val() will get you the result that would be expected Nodes/test_project_file_7ed30cb41d.yaml. But whether this is what you want may be debatable.

--- original answer:

Non-printable characters are ignored unless they surround YAML block style markup (ie : , - or newlines) in such a way that the parser is confused. If they occur inside scalars or within flow style, they will be understood as part of the scalar.

As for the crash you refer, I am not clear on what exactly is the problem. It would help if you provide a MVE in C++ code that shows the problem. I'm having trouble looking at the hex file. If you can show the file encoded as a string, it will make it easier for me to pinpoint what's going on.

[biojppm]

Actually the particular characters do not matter at all, even if non-printable. What matters is this: if the characters come directly after the : character at the end of Nodes:, then the parser will indeed consider the : as part of the scalar. Eg:

Project:
 Name: test_project_file_7ed30cb41d
 Nodes:abc
 - 󠁓File: Nodes/test_project_file_7ed30cb41d.yaml

The scalar will be read starting as Nodes:abc, because : is a valid character inside a scalar. The problem here seems to be that the leading - File on the next line then should cause a parse error (because there was no interleaving occurrence of a token matching the regex :\ *#*.*$, and - would open a seq on a map scope), so the parsing actually succeeds where it shouldn't.

It also explains the later crash upon looking up tree["Nodes"]. As explained above, the (faulty) scalar will start with Nodes:abc, so Nodes does will not exist.

[winneymj]

Hi biojppm,
Here is what I have discovered. It appears the Error callback is not being called when I build in Release, but it does if I build with Debug.
I do receive a callback from the RYML_ASSERT(ch != NONE); when I build in debug.
Is it the case that release builds do not execute the callback or the RYML_USE_ASSERT is not defined.

    NodeRef const operator[] (csubstr const& k) const
    {
        RYML_ASSERT( ! is_seed());
        RYML_ASSERT(valid());
        size_t ch = m_tree->find_child(m_id, k);
        RYML_ASSERT(ch != NONE);
        NodeRef const r(m_tree, ch);
        return r;
    }

I have attached example Visual Studio 2022 project if that helps.
yamlcrash.zip

[winneymj]

I guess what I am saying above is that the line

auto const fileNode = node["File"];

when run in debug does produce an error and callback is executed.
If I run in release no callback is executed and sometimes the program crashes later using the val() from the fileNode.

[biojppm]

RYML_ASSERT() works just like assert() (and can be enabled/disabled with use of NDEBUG, and further with RYML_USE_ASSERT). It is an internal development tool to verify assumptions and preconditions. It will incur a significant runtime cost. Also, it is not meant to verify integrity of the data.

A parse error is a different thing, and should always result in a call to the error callback; if it doesn't it is a bug, and should be reported.

In your case, the YAML parse was successful, and it is only after this phase, when you're going through the data, that the assertion fires because you are accessing a non-existent element. So trying to work around the problem by enabling/disabling assertions is not addressing the root cause: is the YAML valid, and should the element exist? From what I saw in the file, it looks like the YAML is valid, but it's hard for me to be sure because the bytes you mentioned are not occurring after Nodes: , but after - , where they become valid and end up as part of the scalar.

If you want concrete help, please provide a MVE here as a C++ snippet (not a VS project, I am using linux and I can't open that project.)

[winneymj]

Hi, You asked "is the YAML invalid?" A good question. My assumption was no as they are non-printable characters, but I am not a YAML expert and if the YAML specification allows for non-printable chars after a - then it would be valid. Please elaborate. What is a MVE? I assume a virtual environment of some sort?

…

On Fri, Sep 2, 2022 at 12:10 PM jpmag ***@***.***> wrote: RYML_ASSERT() works just like assert() (and can be enabled/disabled with use of NDEBUG, and further with RYML_USE_ASSERT). It is an internal development tool to verify assumptions and preconditions. It will incur a significant runtime cost. Also, it is not meant to verify integrity of the data. A parse error is a different thing, and should always result in a call to the error callback; if it doesn't it is a bug, and should be reported. In your case, the YAML parse was successful, and it is only after this phase, when you're going through the data, that the assertion fires because you are accessing a non-existent element. So trying to work around the problem by enabling/disabling assertions is not addressing the root cause: is the YAML invalid? From what I saw in the file, it looks like the YAML is valid, but it's hard for me to be sure because the bytes you mentioned are not occurring after Nodes: , but after - , where they become valid and end up as part of the scalar. If you want concrete help, please provide a MVE here as a C++ snippet (not a VS project, I am using linux and I can't open that project.) — Reply to this email directly, view it on GitHub <#300 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABK26BAFGDPYZEFHIK7CFZLV4IYIXANCNFSM6AAAAAAQCXRRYI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[biojppm]

MVE stands for minimum verifiable example. It's the least amount of code that reproduces the problem.

[winneymj]

Hi,
I have created an MVE and attached it.
When I am running the sample in debug I am seeing the ErrorCallback being called when trying to find the "File" node.
When I run in release this does not happen. I would think it should be consistent in debug and release.

In my sample it seems stable (no exceptions)....but when run inside our product it comes back with a corrupted "File" node and crashes trying to dereference the call to .val().
Sometimes .val() comes back with corrupted string and then crashes in GetFromString().
I wonder if the optimization is causing some of the different locations for the crash.

main.zip

[biojppm]

For the benefit of other users, please provide the MVE as a small inline code sample on this question. For example, I'm writing this on my phone and it is really hard for me to open the zip and browse through it; so I will not open it. Also, a zip file of a project in fact suggests that the example is not minimal at all.

And to be very clear: the reason for the different behaviour in Debug vs Release is that this is a failed assertion. Assertions are only enabled in Debug builds, or via explicit opt-in as described above. The failed assertion is telling you that something is wrong in what you're trying to achieve, and is there to help you pinpoint the problem in Debug builds. It fails just the same in Release, but it fails silently, and the garbage you are seeing is also telling you the same thing: there's an error in what you're trying to do.

The error may still be caused by a bug in ryml, and that's why I'm still here trying to help you identify the problem. But disabling assertions is definitely not the way to go about it.

[biojppm]

For a good example of an inline MVE, see #283

[perlpunk]

To me it looks like the character isn't a non-printable one, but https://www.compart.com/en/unicode/U+E0053 "Tag Latin Capital Letter S" 0xF3 0xA0 0x81 0x93

[winneymj]

Hi,
Now that you mention that I understand.
I believe I can check for a null ptr

auto const fileNode = node["File"]
if (fileNode == nullptr)
  //Do something here.