Re: [PATCH v1 04/10] tools: add kfuzztest-bridge utility

[Alexander Potapenko]

> ---
> v3:

Please change the version number to something like "RFC v3" (here and
in other patches)


> +
> +static int invoke_one(const char *input_fmt, const char *fuzz_target, const char *input_filepath)
> +{
> +       struct ast_node *ast_prog;
> +       struct byte_buffer *bb;
> +       struct rand_stream *rs;
> +       struct token **tokens;
> +       size_t num_tokens;
> +       size_t num_bytes;
> +       int err;
> +
> +       err = tokenize(input_fmt, &tokens, &num_tokens);
> +       if (err) {
> +               fprintf(stderr, "tokenization failed: %s\n", strerror(-err));
> +               return err;
> +       }

You should be freeing `tokens` somewhere.

> +
> +       err = parse(tokens, num_tokens, &ast_prog);
> +       if (err) {
> +               fprintf(stderr, "parsing failed: %s\n", strerror(-err));
> +               return err;
> +       }
> +
> +       rs = new_rand_stream(input_filepath, 1024);

You need to bail out here if `rs` is NULL, otherwise encode() will crash.

> +       err = encode(ast_prog, rs, &num_bytes, &bb);

`ast_prog` also needs to be freed at the end of this function.

> +int main(int argc, char *argv[])
> +{
> +       if (argc != 4) {
> +               printf("Usage: %s <input-description> <fuzz-target-name> <input-file>\n", argv[0]);
> +               printf("For more detailed information see /Documentation/dev-tools/kfuzztest.rst\n");

This should be Documentation/dev-tools/kfuzztest.rst without the leading slash.

> +static int read_minalign(struct encoder_ctx *ctx)
> +{
> +       const char *minalign_file = "/sys/kernel/debug/kfuzztest/_config/minalign";
> +       char buffer[64 + 1];
> +       int count = 0;
> +       int ret = 0;
> +
> +       FILE *f = fopen(minalign_file, "r");
> +       if (!f)
> +               return -ENOENT;
> +
> +       while (fread(&buffer[count++], 1, 1, f) == 1)
> +               ;

What's the point of this loop, why can't you read sizeof(buffer)-1
bytes instead?
(note that the loop also does not validate the buffer size when reading).

> +       buffer[count] = '\0';
> +
> +       /*
> +        * atoi returns 0 on error. Since we expect a strictly positive
> +        * minalign value on all architectures, a return value of 0 represents
> +        * a failure.
> +        */
> +       ret = atoi(buffer);
> +       if (!ret) {
> +               fclose(f);
> +               return -EINVAL;
> +       }
> +       ctx->minalign = atoi(buffer);

Why are you calling atoi() twice?


> +       ret = malloc(sizeof(*ret));
> +       if (!ret)
> +               return -ENOMEM;
> +       ret->type = NODE_LENGTH;
> +       ret->data.length.length_of = strndup(len->data.identifier.start, len->data.identifier.length);

This strndup() call may fail.


> +       if (!consume(p, TOKEN_RBRACE, "expected '}'") || !consume(p, TOKEN_SEMICOLON, "expected ';'")) {
> +               err = -EINVAL;
> +               goto fail;
> +       }
> +
> +       ret->type = NODE_REGION;
> +       *node_ret = ret;
> +       return 0;
> +
> +fail:

parse_type() may allocate strings using strndup(), which also need to
be cleaned up here.