[Spice-devel] [PATCH spice-server v2] gstreamer: Check if ORC library can work

[Frediano Ziglio]

> 
> 
> > On 25 Aug 2017, at 16:37, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> >>> 
> >>> 
> >>> On Fri, Aug 18, 2017 at 12:32:12PM +0100, Frediano Ziglio wrote:
> >>>> ORC lirabry is used internally by GStreamer to generate code
> >>>> dynamically.
> >>> 
> >>> "The ORC library"
> >>> 
> >>>> If ORC cannot allocate executable memory the failure cause
> >>>> an abort(3) to be called.
> >>> 
> >>> "memory, the failure causes"
> >>> 
> >>>> This happens on some SELinux configuration that disable executable
> >>>> memory allocation (execmem boolean).
> >>> 
> >>> Either "SELinux configurations" or "that disables"
> >>> 
> >>>> Check that ORC could work before attempting to use GStreamer to
> >>>> avoid crashes. The log will report an error.
> >>> 
> >>>> 
> >>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> >>>> ---
> >>>> configure.ac               |  6 ++++++
> >>>> server/Makefile.am         |  2 ++
> >>>> server/gstreamer-encoder.c | 30 ++++++++++++++++++++++++++++++
> >>>> 3 files changed, 38 insertions(+)
> >>>> 
> >>>> Changes since v1:
> >>>> - rename orc_cached to orc_dynamic_code_ok.
> >>>> 
> >>>> diff --git a/configure.ac b/configure.ac
> >>>> index e1e74862..4bdf8588 100644
> >>>> --- a/configure.ac
> >>>> +++ b/configure.ac
> >>>> @@ -116,6 +116,12 @@ AS_IF([test x"$missing_gstreamer_elements" = xyes],
> >>>>     [SPICE_WARNING([The GStreamer video encoder can be built but may not
> >>>>     work.])
> >>>> ])
> >>>> 
> >>>> +if test "x$have_gstreamer_0_10" = "xyes" -o "x$have_gstreamer_1_0" =
> >>>> "xyes"; then
> >>>> +    PKG_CHECK_MODULES(ORC, orc-0.4)
> >>>> +    AC_SUBST(ORC_CFLAGS)
> >>>> +    AC_SUBST(ORC_LIBS)
> >>>> +fi
> >>>> +
> >>>> AC_ARG_ENABLE([automated_tests],
> >>>>               AS_HELP_STRING([--enable-automated-tests], [Enable
> >>>>               automated
> >>>>               tests using spicy-screenshot (part of spice-gtk)]),,
> >>>>               [enable_automated_tests="no"])
> >>>> diff --git a/server/Makefile.am b/server/Makefile.am
> >>>> index ef8d31fc..5d5590af 100644
> >>>> --- a/server/Makefile.am
> >>>> +++ b/server/Makefile.am
> >>>> @@ -20,6 +20,7 @@ AM_CPPFLAGS =					\
> >>>> 	$(SSL_CFLAGS)				\
> >>>> 	$(VISIBILITY_HIDDEN_CFLAGS)		\
> >>>> 	$(WARN_CFLAGS)				\
> >>>> +	$(ORC_CFLAGS)				\
> >>>> 	$(NULL)
> >>>> 
> >>>> noinst_LTLIBRARIES = libserver.la
> >>>> @@ -54,6 +55,7 @@ libserver_la_LIBADD =							\
> >>>> 	$(SSL_LIBS)							\
> >>>> 	$(Z_LIBS)							\
> >>>> 	$(SPICE_NONPKGCONFIG_LIBS)					\
> >>>> +	$(ORC_LIBS)							\
> >>>> 	$(NULL)
> >>>> 
> >>>> libspice_serverincludedir = $(includedir)/spice-server
> >>>> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> >>>> index bb4f27ba..2a39154f 100644
> >>>> --- a/server/gstreamer-encoder.c
> >>>> +++ b/server/gstreamer-encoder.c
> >>>> @@ -27,6 +27,7 @@
> >>>> #include <gst/app/gstappsrc.h>
> >>>> #include <gst/app/gstappsink.h>
> >>>> #include <gst/video/video.h>
> >>>> +#include <orc/orcprogram.h>
> >>>> 
> >>>> #include "red-common.h"
> >>>> #include "video-encoder.h"
> >>>> @@ -1702,6 +1703,30 @@ static void
> >>>> spice_gst_encoder_get_stats(VideoEncoder
> >>>> *video_encoder,
> >>>>     }
> >>>> }
> >>>> 
> >>>> +/* Check if ORC library can work.
> >>>> + * ORC library is used quite extensively by GStreamer
> >>>> + * to generate code dynamically. If ORC cannot work GStreamer
> >>> 
> >>> "cannot work, GStreamer"
> >>> 
> >>>> + * will abort(3) the entire process.
> >>>> + */
> >>>> +static bool orc_check(void)
> >>>> +{
> >>>> +    static bool orc_checked = false;
> >>>> +    static bool orc_dynamic_code_ok = false;
> >>>> +
> >>>> +    if (SPICE_UNLIKELY(!orc_checked)) {
> >>>> +        OrcCode *code = orc_code_new();
> >>>> +        if (code) {
> >>>> +            /* allocating 0 byte for code make the function not crash
> >>> 
> >>> "makes"
> >>> 
> >>>> +             * but doing all initializations and checks */
> >>> 
> >>> I would use "but it does all the initializations and checks"
> >>> 
> >>> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> >>> 
> >> 
> >> I'll do the changes.
> >> 
> >> I just noted that if the check fails is quite silence.
> >> Maybe the first time we discover the issue we should log
> >> something?
> >> 
> > 
> > Ignore that, I just remembered that ORC itself print a huge
> > error.
> 
> Does it even in the case it does not abort() ?
> 
> I see this message in orccodemem.c:
> 
>   ORC_ERROR("Failed to create write and exec mmap regions.  This "
>       "is probably because SELinux execmem check is enabled (good) "
>       "and $TMPDIR and $HOME are mounted noexec (bad).");
> 
> Is that the one?
> 
> Is it possible to add a message on how to fix for our specific use case?
> 
> Thanks
> Christophe
> 
> 

I suppose we can add a g_warning or similar in orc_check telling
the user how can be fixed (like disabling that particular SELinux
flag). Should we put a link to the bugzilla too?
Suggestions for the message (better in patch format) are welcome.

Frediano