On 21/11/16 18:27, Pino Toscano wrote:> 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.
Just one question: shall the changes in upload.c be in a separate commit
"expose XYZ"? What is the preferred way?
This question applies also for the notes in PATCH 5.>
>> +/* 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,
>
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs