[Libguestfs] [PATCH 2/6] New API: yara_load

[Pino Toscano]

On Wednesday, 2 November 2016 21:26:20 CET Matteo Cafasso wrote:
> The yara_load API allows to load a set of Yara rules contained within a
> file on the host.
> 
> Rules can be in binary format, as when compiled with yarac command, or
> in source code format. In the latter case, the rules will be first
> compiled and then loaded.

This paragraph may be worth added in the API longdesc as well, since it
gives more hint to users.

> +/* Yara compiled rules. */
> +static YR_RULES *rules = NULL;

This is not properly freed at shutdown -- take a look at what is done
for Augeas, aug_finalize.

> +static int upload_rules_file (char *);
> +static int compile_rules_file (const char *);
> +static int write_callback (void *, const void *, size_t);
> +
> +/* Has one FileIn parameter. */
> +int
> +do_yara_load (void)
> +{
> +  int ret = 0;
> +  char tmpfile[] = "/tmp/yaraXXXXXX";
> +
> +  ret = upload_rules_file (tmpfile);
> +  if (ret < 0)
> +    return -1;
> +
> +  ret = yr_initialize ();

The libyara API docs do not say whether this is doable multiple times,
so I'd do that only once at all.

> +  if (ret != ERROR_SUCCESS) {
> +    reply_with_error ("failed initializing yara");
> +    unlink (tmpfile);
> +    return -1;
> +  }
> +
> +  /* Destroy previously loaded rules. */
> +  if (rules != NULL) {
> +    yr_rules_destroy (rules);
> +    rules = NULL;
> +  }
> +
> +  /* Try to load the rules as compiled.
> +   * If their are in source code format, compile them first.
> +   */
> +  ret = yr_rules_load (tmpfile, &rules);
> +  if (ret == ERROR_INVALID_FILE)
> +    ret = compile_rules_file (tmpfile);
> +
> +  unlink (tmpfile);
> +
> +  if (ret != ERROR_SUCCESS) {
> +    reply_with_error ("failed loading the rules");
> +    yr_finalize ();
> +    return -1;
> +  }
> +
> +  ret = yr_finalize ();

Accoding to the API documentation, yr_finalize "must be called when
you are finished using the library". OTOH, libyara is used also after
this point (by other APIs, for example), so we cannot use yr_finalize
here.

> +  if (ret != ERROR_SUCCESS) {
> +    reply_with_error ("failed finalizing yara");
> +    return -1;
> +  }
> +
> +  return 0;
> +}
> +
> +/* Upload rules file on a temporary file.
> + * Return 0 on success, -1 on error.
> + */
> +static int
> +upload_rules_file (char *rules_path)
> +{
> +  int ret = 0;
> +  CLEANUP_CLOSE int fd = 0;
> +  struct write_callback_data data = { .written = 0 };
> +
> +  data.fd = mkstemp (rules_path);
> +  if (data.fd == -1) {
> +    reply_with_perror ("mkstemp");
> +    return -1;
> +  }
> +
> +  ret = receive_file (write_callback, &data.fd);

'fd' happens to be the first member in the write_callback_data struct,
so this passes the right address for the callback. OTOH, passing the
address of the struct itself is a better option, so order changes in
the members of the struct won't cause the callback to explode.

> +  if (ret == -1) {
> +    /* Write error. */
> +    cancel_receive ();
> +    reply_with_error ("write error: %s", rules_path);
> +    return -1;
> +  }
> +  if (ret == -2) {
> +    /* Cancellation from library */
> +    reply_with_error ("file upload cancelled");
> +    return -1;
> +  }
> +
> +  return 0;
> +}
> +
> +/* Compile source code rules and load them.
> + * Return ERROR_SUCCESS on success, Yara error code type on error.
> + */
> +static int
> +compile_rules_file (const char *rules_path)
> +{
> +  int ret = 0;
> +  FILE *rule_file = NULL;

Sounds like you could copy the CLEANUP_FCLOSE stuff from the library
to the daemon (see daemon/cleanups.{c,h}).

> +  YR_COMPILER *compiler = NULL;

Likewise, adding a custom CLEANUP_YR_COMPILER would help making this
function less leak prone.

> +  ret = yr_compiler_create (&compiler);
> +  if (ret != ERROR_SUCCESS) {
> +    perror ("yr_compiler_create");

I'd issue reply_with_(p)error instead of perror directly in this
function, avoiding generic error messages in do_yara_load.

> +    return ret;
> +  }
> +
> +  rule_file = fopen (rules_path, "r");
> +  if (rule_file == NULL) {
> +    yr_compiler_destroy (compiler);
> +    perror ("unable to open rules file");
> +    return ret;
> +  }
> +
> +  ret = yr_compiler_add_file (compiler, rule_file, NULL, rules_path);
> +  if (ret > 0) {
> +    yr_compiler_destroy (compiler);
> +    fclose (rule_file);
> +    perror ("yr_compiler_add_file");

It looks like there is yr_compiler_* API for getting more information
on the actual error -- please make use of it in error message, so it's
easier to know why something failed.

Thanks,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20161107/40f170c9/attachment.sig>