unsafe usage of evt_tag_errno

[furiel]

There are evt_tag_errno calls all around the code that use the errno variable directly. In some cases it is not safe, because other evt_tags in msg_(error/warning/etc) can call functions that can overwrite errno. For example in the snippet below, either malloc or strdrup in evt_tag_str can overwrite errno.

As compiler is free to evaluate parameters in any order, it is not enough to move evt_tag_errno to the first parameter.

      cap_text = cap_to_text(caps, NULL);
      msg_error("Error managing capability set, cap_set_proc returned an error",
                evt_tag_str("caps", cap_text),
                evt_tag_errno("error", errno));

The resolution would be to store errno on the stack, and pass the local variable to errno.

As a bonus: evt_tag_errno might disallow users to pass errno directly: for example compare if &errno is passed to it, and assert in that case. G_LIKELY/UNLIKELY can be used inside the if statement for optimization.

As an estimate, 64/76 of evt_tag_errno uses errno directly.

$ ag evt_tag_errno | grep -F "errno)" | wc -l
64
$ ag evt_tag_errno | wc -l
76

[bazsi]

I think we should rather make sure that evtlog is errno clean. I think that would be easier, especially as we can simply abort if malloc fails, as we do with glib based allocations.

…

-- Bazsi

On Sun, Apr 15, 2018 at 12:11 PM, furiel ***@***.***> wrote: There are evt_tag_errno calls all around the code that use the errno variable directly. In some cases it is not safe, because other evt_tags in msg_(error/warning/etc) can call functions that can overwrite errno. For example in the snippet below, either malloc or strdrup in evt_tag_str can overwrite errno. As compiler is free to evaluate parameters in any order, it is not enough to move evt_tag_errno to the first parameter. cap_text = cap_to_text(caps, NULL); msg_error("Error managing capability set, cap_set_proc returned an error", evt_tag_str("caps", cap_text), evt_tag_errno("error", errno)); The resolution would be to store errno on the stack, and pass the local variable to errno. As a bonus: evt_tag_errno might disallow users to pass errno directly: for example compare if &errno is passed to it, and assert in that case. G_LIKELY/UNLIKELY can be used inside the if statement for optimization. As an estimate, 64/76 of evt_tag_errno uses errno directly. $ ag evt_tag_errno | grep -F "errno)" | wc -l 64 $ ag evt_tag_errno | wc -l 76 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1990>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArldm_qYgx_QvxOEqma0760xUTfejXcks5toxzYgaJpZM4TVag1> .

[furiel]

Abort on allocation problems is unfortunately not enough: due to evt_tag_printf, users can involve any kind of calls that dynamically captures errno, and not related to allocation errors. For examle in evt_tag_str(..., g_sockaddr_format(...)), we will implicitely call inet_ntop which can capture errno.

But I agree it would be best if we could hide this detail inside evtlog. I am interested in this, let me try to do some macromagic: see if I can lexically capture errno, store it in a gensym, and make evt_tag_errno expand into a call that receives the captured errno in the gensym. I might even be able to maintain api compatibility: evt_tag_errno might check if user passed an errno, and in that case, go with it instead of the gensym.