OSS-fuzz

[AdamKorcz]

The fuzzer for ucl_parser_add_string runs into a bug very quickly when fuzzing the function:

libucl/src/ucl_parser.c

Lines 3049 to 3059 in 91a3fb4

	bool
	ucl_parser_add_string (struct ucl_parser *parser, const char *data,
	size_t len)
	{
	if (parser == NULL) {
	return false;
	}
	return ucl_parser_add_string_priority (parser,
	(const unsigned char *)data, len, parser->default_priority);
	}

This function accepts a length argument, however, the following function:

libucl/src/ucl_parser.c

Lines 3033 to 3047 in 91a3fb4

	bool
	ucl_parser_add_string_priority (struct ucl_parser *parser, const char *data,
	size_t len, unsigned priority)
	{
	if (data == NULL) {
	ucl_create_err (&parser->err, "invalid string added");
	return false;
	}
	if (len == 0) {
	len = strlen (data);
	}
	return ucl_parser_add_chunk_priority (parser,
	(const unsigned char *)data, len, priority);
	}

may cause a buffer overflow if the len argument is 0 and the data string is not null-terminated.

I suggest we do one of two things:

• Remove the use of strlen and len == 0 all together and simply remove the lines (3041-3043)
• Make it explicit that if len == 0 then the string is expected to be null-terminated and otherwise undefined behavior may occur

The OSS-Fuzz build is currently blocking because the bug is found too quickly:
https://travis-ci.org/github/google/oss-fuzz/jobs/670351676?utm_medium=notification&utm_source=github_status

[vstakhov]

Yes, assuming that len == 0 implies zero terminated string was a mistake. However, I'd prefer the second option as it will not break the existing API and its usage. I would like to go with the first option if compatibility was not important tbh...