Fix parsing arg by forzafedor · Pull Request #14 · ligurio/luzer

forzafedor · 2023-12-19T16:46:45Z

I changed the flag formatting algorithm for libfuzzer

2 problems were solved:

Now all flags are processed correctly and the libfuzzer receives the file name as a zero argument. (libfuzzer receives flag from first index link).
Now no error with free() function when script finishes work (there was an error in the algorithm of argument parsing).

Now flags (and corpus directories) for libfuzzer can be added from console.
For example:

lua example_basic.lua -max_total_time=60 -only_ascii=1
lua example_basic.lua ./corpus/

But now for luzer.Fuzz() function as third arguments need use global table arg.
If you need to add flags from a Lua script, you can use the construction:

local args = {
    "-only_ascii=1",
}

for i = 1, #args do
    arg[#arg + 1] = args[i]
end

luzer.Fuzz(TestOneInput, nil, arg)

Perhaps there is a better and easier way :)

forzafedor · 2023-12-20T15:33:19Z

Don't look at the merge yet, I'll change it

ligurio

Thanks for the patch!

I left a number of comments, please take a look.
Also, please squash commits to a single one, update commit description (it is better to say that we change arg format, not algorithm, and add a long commit description). Add Fixes #13 at the end of commit message`. Changelog entry is also required.

luzer/tests/test_options.lua

ligurio · 2023-12-24T09:08:12Z

docs/usage.md

@@ -32,7 +32,7 @@ local function TestOneInput(buf)
    parser.parse(decoder.decode(buf))


BTW docs/api.md should be updated too.
At least this:

args is a table with arguments: the process arguments to pass to the
fuzzer. Field corpus specifies a path to a directory with seed corpus, see a
list with other options in the [libFuzzer documentation][libfuzzer-options-url].

Not relevant

Please describe passing arguments via command-line in documentation.

documentation is still missed

luzer/luzer.c

ligurio · 2023-12-24T09:25:05Z

README.md

 end

-luzer.Fuzz(TestOneInput)
+luzer.Fuzz(TestOneInput, nil, arg)


arg is added, but in a step 3 it is unused (no options are passed to a script).

examples/example_basic.lua

forzafedor · 2023-12-27T11:45:02Z

Thanks for the review!
I've changed the argument parsing process and added support for command line arguments.
The code from Lua's has not changed

ligurio

Thanks for update!
I like your idea to keep a Lua table with options and merge it with CLI arguments.
Please see the comments inline.

CHANGELOG.md

ligurio · 2023-12-28T06:46:21Z

README.md

 end

-luzer.Fuzz(TestOneInput)
+luzer.Fuzz(TestOneInput, nil, {})


What for this change?

luzer/luzer/luzer.c

Line 330 in 0179547

if (lua_istable(L, -1) == 0) {

At the moment, it is checked that the last argument is a table and if not, fuzzing ends with an error. Therefore, it is necessary to explicitly specify arguments 2 and 3.

It is not clear why support of CLI arguments makes custom mutator and table with options mandatory for Fuzz function.

The problem is that the code currently shown in the readme will not work at all. The fuzzing process will start only if all 3 arguments are passed to the function input. In this PR, the process of parsing input arguments has been changed, I decided to fix this too.

ligurio · 2023-12-28T07:41:14Z

luzer/luzer.c

 #define CUSTOM_MUTATOR_LIB "libcustom_mutator.so.1"
 #define DEBUG_HOOK_FUNC "luzer_custom_hook"

+#define ENV_NOT_USE_CLI_ARGS "LUZER_NOT_USE_CLI_ARGS_FOR_LF"


According to changelog, option allows discarding arguments specified in command line.
What is the point? Just don't pass these options, and that's all. Or I didn't get your usecase.
From my point of view, it is more sense to introduce an option that will allow ignoring libfuzzer options specified in Lua.

This is necessary so that if we fuzz functionality that works with command line arguments, there may be problems if the flags are the same.
For example:

local function TestOneInput(buf) local fdp = luzer.FuzzedDataProvider(buf) local str = fdp:consume_string(4) local b = {} str:gsub(".", function(c) table.insert(b, c) end) if (b[1] == "o" and #arg > 0 and arg[1] == "-help=1") then assert(nil) end end

if we need to set the "-help=1" flag, then without this environment variable, the fuzzer will simply launch its help and fuzzing will not even start, but with the variable set, this crash will be found.

luzer/luzer.c

ligurio · 2023-12-28T07:54:00Z

luzer/luzer.c

+        if (result < 0)
+                luaL_error(L, "failed to parse arguments from lua");


According to implementation get_args_from_lua never returns non-zero exit code. It is a Lua C function and we raise lua_error inside it.

Add descriptions for luaL_get_args_from_cli and luaL_get_args_from_table with possible return codes.

ligurio · 2023-12-28T07:59:43Z

luzer/luzer.c

+        if (result < 0)
+                luaL_error(L, "failed to merge arguments");


Message "failed to merge arguments" says nothing to user. It is not clear what a problem is happened and what is "merging arguments" at all. Probably it is better to return an error code for each type of error and use this code here. Currently, we return non-zero code in merge_args in case of lack of memory.

ligurio · 2023-12-28T08:03:42Z

luzer/luzer.c

+        if (result < 0)
+                luaL_error(L, "failed to parse arguments from console");


same comment as for get_args_from_lua

luzer/luzer.c

forzafedor · 2023-12-29T10:45:03Z

Corrected the PR according to your comments

luzer/luzer_args.c

ligurio · 2023-12-29T13:10:21Z

luzer/luzer_args.c

+	char search_flag[strlen(key) + 3];
+	snprintf(search_flag, strlen(key) + 3, "-%s=", key);


Please define a var for strlen(key) + 3 and use it in array definition and for snprintf.

ligurio · 2023-12-29T13:11:01Z

luzer/luzer_args.c

+	char search_flag[strlen(key) + 3];
+	snprintf(search_flag, strlen(key) + 3, "-%s=", key);
+	for (int i = 0; i <= f_args->argc; i++) {
+		if (strncmp(f_args->argv[i], search_flag, strlen(search_flag)) == 0)


strlen(search_flag) is known, replace with variable (see a previous comment)

Why ignored? You calculate string length three times!

luzer/luzer_args.c

luzer/luzer.h

ligurio · 2023-12-29T13:31:36Z

luzer/luzer_args.c

+                if (!arg)
+                        return -1;
+
+                if (key > 0 && (not_use_cli_args == NULL || !strncmp(not_use_cli_args, "0", 1))) {


getenv(3):

The getenv() function returns a pointer to the value in the environment, or NULL if there is no match.

so comparing not_use_cli_args with NULL is enough

Do not use console arguments when LUZER_NOT_USE_CLI_ARGS_FOR_LF is set (even when equal to 0) and use console arguments when LUZER_NOT_USE_CLI_ARGS_FOR_LF is not set.

CHANGELOG.md

forzafedor · 2024-01-08T11:02:39Z

@ligurio Wait for your response.

ligurio · 2024-01-09T08:56:10Z

@ligurio Wait for your response.

Sure, I'll take a look.

ligurio

Thanks for fixes!
The patch looks good, need some polishing. Please resolve comments that I left.

luzer/luzer_args.c

luzer/luzer.c

luzer/tests/CMakeLists.txt

ligurio · 2024-01-15T19:35:31Z

README.md

 end

-luzer.Fuzz(TestOneInput)
+luzer.Fuzz(TestOneInput, nil, {})


It is not clear why support of CLI arguments makes custom mutator and table with options mandatory for Fuzz function.

luzer/luzer_args.c

…ents for libfuzzer Fixes ligurio#13

forzafedor · 2024-01-22T08:59:25Z

@ligurio Thanks for the review. I fixed everything

ligurio

Thanks for fixes! We are close to finish, but need a little more work.
Please take a look on comments.

ligurio · 2024-02-01T16:44:05Z

luzer/luzer_args.c

@@ -0,0 +1,284 @@
+/*


Please fix indentation in a whole patch.

ligurio · 2024-02-01T16:46:46Z

docs/usage.md

@@ -32,7 +32,7 @@ local function TestOneInput(buf)
    parser.parse(decoder.decode(buf))


documentation is still missed

examples/example_basic.lua

ligurio · 2024-02-01T16:50:34Z

luzer/luzer_args.c

+		const int key = lua_tointeger(L, -2);
+		lua_pop(L, 1);
+
+                if (key < 0) {


Add comment here with explanation

ligurio · 2024-02-01T16:55:38Z

luzer/luzer.c

+        if (result < 0)
+                luaL_error(L, "failed to parse arguments from lua");


Add descriptions for luaL_get_args_from_cli and luaL_get_args_from_table with possible return codes.

ligurio · 2024-02-01T17:24:03Z

luzer/luzer_args.c

+#include "luzer_args.h"
+#include "macros.h"
+
+#define ENV_NOT_USE_CLI_ARGS "LUZER_NOT_USE_CLI_ARGS"


I would rename ENV_NOT_USE_CLI_ARGS to IGNORE_CLI_ARGS and the same for env variable name: LUZER_NOT_USE_CLI_ARGS -> LUZER_IGNORE_CLI_ARGS.

ligurio · 2024-02-01T17:25:21Z

luzer/luzer_args.c

+#define FLAG_SCANF_FORMAT_KEY "-%[^=]"
+#define FLAG_PATTERN_KEY "-%s="
+#define FLAG_PATTERN_KEY_VALUE "-%s=%s"
+#define FLAG_PATTERN_OVERHEAD 2


Used only once in a line below, I would drop it.

ligurio · 2024-02-01T17:26:12Z

luzer/luzer_args.c

+NO_SANITIZE static bool
+is_flag_in_args(luzer_args *f_args, const char *key) {
+	if (!f_args || !f_args->argv || f_args->argc <= 1) {
+                return false;


Why functions below returns int and here you are using bool?

Do you mean to change return type to int? With return 0/1?

ligurio · 2024-02-01T17:29:23Z

luzer/luzer_args.c

+	total_args->argv[0] = cli_args->argv[0];
+
+	char *corpus_path = NULL;
+	for (int i = 0; i < table_args->argc; i++) {


Probably it is better to put table_args->argv[i] to a separate variable (for example cur_arg) at the beginning of loop iteration. This change will reduce length of lines a bit. Currently, it is difficult to read lines in a loop body.

ligurio · 2024-02-01T17:31:27Z

luzer/luzer_args.c

+	luzer_args table_args = { .argv = NULL, .argc = 0 };
+
+        int result = -1;
+        result = luaL_get_args_from_cli(L, &cli_args);


It is not a self-explained variable name. I propose to rename to something like retcode, rc or something else.

ligurio · 2025-03-19T07:20:31Z

Superseded by #21.

forzafedor mentioned this pull request Dec 19, 2023

Incorrect parsing of Libfuzzer flags #13

Closed

forzafedor marked this pull request as draft December 20, 2023 15:34

ligurio reviewed Dec 24, 2023

View reviewed changes

forzafedor force-pushed the fix-parsing-arg branch from e069f35 to b877c49 Compare December 27, 2023 11:24

forzafedor marked this pull request as ready for review December 27, 2023 13:03

forzafedor force-pushed the fix-parsing-arg branch from b877c49 to d1981f4 Compare December 27, 2023 13:21

ligurio reviewed Dec 28, 2023

View reviewed changes

forzafedor force-pushed the fix-parsing-arg branch from d1981f4 to a0e1cc8 Compare December 29, 2023 10:33

ligurio reviewed Dec 29, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

forzafedor force-pushed the fix-parsing-arg branch from a0e1cc8 to 1643bce Compare January 8, 2024 10:57

ligurio requested changes Jan 15, 2024

View reviewed changes

forzafedor force-pushed the fix-parsing-arg branch 5 times, most recently from d10c7b5 to 746ace8 Compare January 21, 2024 20:17

luzer: fix parse args from lua & added support for command line argum…

e6a2372

…ents for libfuzzer Fixes ligurio#13

forzafedor force-pushed the fix-parsing-arg branch from 746ace8 to e6a2372 Compare January 21, 2024 20:21

ligurio reviewed Feb 1, 2024

View reviewed changes

ligurio closed this Mar 19, 2025

		@@ -32,7 +32,7 @@ local function TestOneInput(buf)
		parser.parse(decoder.decode(buf))

		if (result < 0)
		luaL_error(L, "failed to parse arguments from lua");

		char search_flag[strlen(key) + 3];
		snprintf(search_flag, strlen(key) + 3, "-%s=", key);

Conversation

forzafedor commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forzafedor commented Dec 20, 2023

Uh oh!

ligurio left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forzafedor Dec 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

forzafedor commented Dec 27, 2023

Uh oh!

ligurio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

forzafedor commented Dec 29, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

forzafedor commented Jan 8, 2024

forzafedor commented Dec 19, 2023 •

edited

Loading

ligurio left a comment •

edited

Loading

forzafedor Dec 27, 2023 •

edited

Loading