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

[Christophe de Dinechin]

> On 19 May 2017, at 13:23, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> ORC lirabry is used internally by GStreamer to generate code
> dynamically.
> If ORC cannot allocate executable memory the failure cause
> an abort(3) to be called.
> This happens on some SELinux configuration that disable executable
> memory allocation (execmem boolean).
> 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(+)
> 
> This patch was tested with Fedora 25 and GStreamer 1.0
> and RHEL6.
> Note for packagers: this will require orc-devel dependency
> on the spec file if GStreamer is used.
> 
> diff --git a/configure.ac b/configure.ac
> index 9ebcfd0..2ec021e 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 ef8d31f..5d5590a 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 bb4f27b..25dbd52 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
> + * will abort(3) the entire process.
> + */
> +static bool orc_check(void)
> +{
> +    static bool orc_checked = false;
> +    static bool orc_cached = false;

The name ‘orc_cached’ doesn’t speak to me. What about ‘orc_dynamic_code_ok’ or something a bit more specific like that?

> +
> +    if (SPICE_UNLIKELY(!orc_checked)) {
> +        OrcCode *code = orc_code_new();
> +        if (code) {
> +            /* allocating 0 byte for code make the function not crash
> +             * but doing all initializations and checks */

How confident are you that this behavior will remain in future ORC code evolutions?

(Not saying I have a better suggestion, but this seems a bit fragile)

> +            orc_code_allocate_codemem(code, 0);
> +            orc_cached = code->code != NULL;
> +            orc_code_free(code);
> +        }
> +        orc_checked = true;
> +    }
> +    return orc_cached;
> +}
> +
> VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType codec_type,
>                                     uint64_t starting_bit_rate,
>                                     VideoEncoderRateControlCbs *cbs,
> @@ -1721,6 +1746,11 @@ VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType codec_type,
>         return NULL;
>     }
> 
> +    // avoid aborting the process
> +    if (!orc_check()) {
> +        return NULL;
> +    }
> +
>     SpiceGstEncoder *encoder = spice_new0(SpiceGstEncoder, 1);
>     encoder->base.destroy = spice_gst_encoder_destroy;
>     encoder->base.encode_frame = spice_gst_encoder_encode_frame;
> -- 
> 2.9.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel