[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Wednesday, 9 November 2016 22:38:53 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.
> 
> Subsequent calls of the yara_load API will result in the discard of the
> previously loaded rules.
> 
> Signed-off-by: Matteo Cafasso <noxdafox gmail com>
> ---
> [...]
> diff --git a/daemon/cleanups.c b/daemon/cleanups.c
> index 092e493..a02e521 100644
> --- a/daemon/cleanups.c
> +++ b/daemon/cleanups.c
> @@ -78,3 +93,16 @@ cleanup_free_stringsbuf (void *ptr)
>  {
>    free_stringsbuf ((struct stringsbuf *) ptr);
>  }
> +
> +#ifdef HAVE_YARA
> +
> +void
> +cleanup_destroy_yara_compiler (void *ptr)
> +{
> +  YR_COMPILER *compiler = * (YR_COMPILER **) ptr;
> +
> +  if (compiler != NULL)
> +    yr_compiler_destroy (compiler);
> +}
> +

This should rather be directly in daemon/yara.c, since libyara would be
used there only.

> +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);
> +  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;
> +}

This function does not always unlink the temporary file on error, and
it is never close either -- my suggestion would be to reuse and expose
parts of the "upload" function in daemon/upload.c:

  int upload_to_fd (int fd)

With the above, upload_rules_file could not be needed anymore, and the
logic to open a temporary fd could be moved directly at the beginning
of do_yara_load.

> +/* 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;
> +  CLEANUP_FCLOSE FILE *rule_file = NULL;
> +  CLEANUP_DESTROY_YARA_COMPILER YR_COMPILER *compiler = NULL;
> +
> +  ret = yr_compiler_create (&compiler);
> +  if (ret != ERROR_SUCCESS) {
> +    reply_with_error ("yr_compiler_create");
> +    return ret;
> +  }
> +
> +  yr_compiler_set_callback (compiler, compile_error_callback, NULL);
> +
> +  rule_file = fopen (rules_path, "r");
> +  if (rule_file == NULL) {
> +    reply_with_error ("unable to open rules file");
> +    return ret;
> +  }
> +
> +  ret = yr_compiler_add_file (compiler, rule_file, NULL, rules_path);

Considering it's only a temporary file, and thus we don't show its path
in error message, we could avoid passing rules_path here -- it looks
like the libyara API allows NULL for this parameter.

> +  if (ret > 0)
> +    return ret;
> +
> +  ret = yr_compiler_get_rules (compiler, &rules);
> +  if (ret != ERROR_SUCCESS)
> +    reply_with_error ("yr_compiler_get_rules");

The API doc say the return value can be either ERROR_SUCCESS or
ERROR_INSUFICENT_MEMORY, so I'd map the latter to ENOMEM and use
reply_with_perror.

> +/* Yara compilation error callback.
> + * Reports back the compilation error message.
> + * Prints compilation warnings if verbose.
> + */
> +static void
> +compile_error_callback(int level, const char *name, int line,
> +                       const char *message, void *data)
> +{
> +  if (level == YARA_ERROR_LEVEL_ERROR)
> +    reply_with_error ("(%d): Yara error: %s", line, message);

"Yara error (line %d): %s"

> +  else
> +    fprintf (stderr, "(%d): Yara warning: %s\n", line, message);

Ditto.
In addition, IMHO this message should be shown only when verbose is
true... like what is written in the API doc comment at the beginning
of the function :)

> +}
> +
> +/* Clean up yara handle on daemon exit. */
> +void yara_finalize (void) __attribute__((destructor));
> +void
> +yara_finalize (void)
> +{
> +  int ret = 0;

  if (!initialized)
    return;

> +
> +  if (rules != NULL) {
> +    yr_rules_destroy (rules);
> +    rules = NULL;
> +  }
> +
> +  ret = yr_finalize ();
> +  if (ret != ERROR_SUCCESS)
> +    perror ("yr_finalize");
> +
> +  initialized = false;
> +}

Thanks,
-- 
Pino Toscano

Attachment: signature.asc
Description: This is a digitally signed message part.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]