[virglrenderer-devel] [PATCH 2/2] protect gl{BeginEnd}ConditionalRendering calls

[Po-Hsien Wang]

Checking fill_caps, which I believe is the point of
truth, caps->v1.bset.conditional_render followed the fill_caps_gles and set
to false if gles is detected.
Why we have two different path for filling the have_ and caps?

On Wed, Jul 18, 2018 at 1:12 PM David Riley <davidriley at chromium.org> wrote:

> On Wed, Jul 18, 2018 at 1:10 PM Po-Hsien Wang <pwang at chromium.org> wrote:
>
>> Shouldn't have_gl_conditional_render deps on !gles?
>>
>
> To expand on this, the change as posted still fails for me with the same
> fuzzing input, but adding a !gles check to the have_gl_conditional_render
> assignment fixed the failure in question.
>
>
>>
>> On Wed, Jul 18, 2018 at 1:04 AM Erik Faye-Lund <
>> erik.faye-lund at collabora.com> wrote:
>>
>>> Otherwise, it'd be possible to generate evil commands from a rouge
>>> guest-driver that can crash the VM.
>>>
>>> This is a bit trickier than the previous one, because we were already
>>> mixing calls to the OpenGL 3.0 version and the GL_NV_conditional_render
>>> version, which indicates that this was previously only safe if *both*
>>> were supported, that is OpenGL 3.0 *with* GL_NV_conditional_render. Now
>>> this code should match the caps we generate, which shouldn't give any
>>> percieved feature-regressions or gl-versions supported.
>>>
>>> Signed-off-by: Erik Faye-Lund <erik.faye-lund at collabora.com>
>>> ---
>>> So this is a little bit tric
>>>
>>>  src/vrend_renderer.c | 30 ++++++++++++++++++++++++------
>>>  1 file changed, 24 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
>>> index 564807a..f34a58f 100644
>>> --- a/src/vrend_renderer.c
>>> +++ b/src/vrend_renderer.c
>>> @@ -109,6 +109,8 @@ struct global_renderer_state {
>>>     bool have_ms_scaled_blit;
>>>     bool have_nv_prim_restart;
>>>     bool have_gl_prim_restart;
>>> +   bool have_gl_conditional_render;
>>> +   bool have_nv_conditional_render;
>>>     bool have_bit_encoding;
>>>     bool have_gles31_vertex_attrib_binding;
>>>     bool have_tf2;
>>> @@ -4523,6 +4525,10 @@ int vrend_renderer_init(struct vrend_if_cbs *cbs,
>>> uint32_t flags)
>>>        vrend_state.have_gl_prim_restart = true;
>>>     else if (epoxy_has_gl_extension("GL_NV_primitive_restart"))
>>>        vrend_state.have_nv_prim_restart = true;
>>> +   if (gl_ver >= 30)
>>> +      vrend_state.have_gl_conditional_render = true;
>>> +   else if (epoxy_has_gl_extension("GL_NV_conditional_render"))
>>> +      vrend_state.have_nv_conditional_render = true;
>>>     if (gl_ver >= 40 || (gles && gl_ver >= 30) ||
>>>         epoxy_has_gl_extension("GL_ARB_transform_feedback2")) {
>>>        vrend_state.have_tf2 = true;
>>> @@ -7008,11 +7014,18 @@ static void vrend_pause_render_condition(struct
>>> vrend_context *ctx, bool pause)
>>>  {
>>>     if (pause) {
>>>        if (ctx->sub->cond_render_q_id)
>>> -         glEndConditionalRenderNV();
>>> +         if (vrend_state.have_gl_conditional_render)
>>> +            glEndConditionalRender();
>>> +         else if (vrend_state.have_nv_conditional_render)
>>> +            glEndConditionalRenderNV();
>>>     } else {
>>>        if (ctx->sub->cond_render_q_id)
>>> -         glBeginConditionalRender(ctx->sub->cond_render_q_id,
>>> -                                  ctx->sub->cond_render_gl_mode);
>>> +         if (vrend_state.have_gl_conditional_render)
>>> +            glBeginConditionalRender(ctx->sub->cond_render_q_id,
>>> +                                     ctx->sub->cond_render_gl_mode);
>>> +         else if (vrend_state.have_nv_conditional_render)
>>> +            glBeginConditionalRenderNV(ctx->sub->cond_render_q_id,
>>> +                                       ctx->sub->cond_render_gl_mode);
>>>     }
>>>  }
>>>
>>> @@ -7025,7 +7038,10 @@ void vrend_render_condition(struct vrend_context
>>> *ctx,
>>>     GLenum glmode = 0;
>>>
>>>     if (handle == 0) {
>>> -      glEndConditionalRenderNV();
>>> +      if (vrend_state.have_gl_conditional_render)
>>> +         glEndConditionalRender();
>>> +      else if (vrend_state.have_nv_conditional_render)
>>> +         glEndConditionalRenderNV();
>>>        ctx->sub->cond_render_q_id = 0;
>>>        ctx->sub->cond_render_gl_mode = 0;
>>>        return;
>>> @@ -7054,8 +7070,10 @@ void vrend_render_condition(struct vrend_context
>>> *ctx,
>>>
>>>     ctx->sub->cond_render_q_id = q->id;
>>>     ctx->sub->cond_render_gl_mode = glmode;
>>> -   glBeginConditionalRender(q->id, glmode);
>>> -
>>> +   if (vrend_state.have_gl_conditional_render)
>>> +      glBeginConditionalRender(q->id, glmode);
>>> +   if (vrend_state.have_nv_conditional_render)
>>> +      glBeginConditionalRenderNV(q->id, glmode);
>>>  }
>>>
>>>  int vrend_create_so_target(struct vrend_context *ctx,
>>> --
>>> 2.18.0.rc2
>>>
>>> _______________________________________________
>> virglrenderer-devel mailing list
>> virglrenderer-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/virglrenderer-devel/attachments/20180718/cb2d262c/attachment-0001.html>