1712640 - Coverity OVERRUN warning in third_party/rust/glslopt/glsl-optimizer/src/compiler/glsl/link_uniform_initializers.cpp

Status: NEW

(Core :: Graphics: WebRender, defect)

Description

[Daniel Veditz [:dveditz]]

Thanks to Jesup for flagging this to me: We got a Coverity scan warning on May 23, 2021 of a new "OVERRUN" detection in the glslopt code . Not quite sure why it's "new" since we updated that crate version in March (IIRC), but maybe something recently turned it on

The warning is accurate, but probably not a security problem assuming no one ever removes the GLfloat from the union. But it would be safer to add a GLuint64 member to the union and use that here instead.

I don't know where the ultimate source for this belongs. Our "third-party" code comes from a crate owned by jnicol. The header that defines this union is in mesa code incorporated directly into the crate, but probably has an upstream that needs updating.

/third_party/rust/glslopt/glsl-optimizer/src/compiler/glsl/link_uniform_initializers.cpp: 70 in linker::copy_constant_to_storage(gl_constant_value *, const ir_constant *, glsl_base_type, unsigned int, unsigned int)()
64              storage[i].f = val->value.f[i];
65              break;
66           case GLSL_TYPE_DOUBLE:
67           case GLSL_TYPE_UINT64:
68           case GLSL_TYPE_INT64:
69              /* XXX need to check on big-endian */
>>>     CID 1451261:  Memory - corruptions  (OVERRUN)
>>>     Overrunning buffer pointed to by "&storage[i * 2U].u" of 4 bytes by passing it to a function which accesses it at byte offset 7 using argument "8UL".
70              memcpy(&storage[i * 2].u, &val->value.d[i], sizeof(double));
71              break;
72           case GLSL_TYPE_BOOL:
73              storage[i].b = val->value.b[i] ? boolean_true : 0;
74              break;
75           case GLSL_TYPE_ARRAY:```

The union is defined at https://searchfox.org/mozilla-central/source/third_party/rust/glslopt/glsl-optimizer/src/mesa/program/prog_parameter.h#80-86

Comment 1

[Daniel Veditz [:dveditz]]

needinfo to jnicol to check my assumptions about safety here.

Comment 2

[Jamie Nicol [:jnicol]]

The warning is accurate, but probably not a security problem assuming no one ever removes the GLfloat from the union. But it would be safer to add a GLuint64 member to the union and use that here instead.

I'm not sure I understand how the GLfloat helps.

If you look at the calling code, it seems they are quite deliberately packing 64bit types in to 2 elements of the array: https://searchfox.org/mozilla-central/source/third_party/rust/glslopt/glsl-optimizer/src/compiler/glsl/link_uniform_initializers.cpp#227-237

I could believe this is unsafe. (I'm not an expert at what is and isn't defined behaviour in C/C++). But changing the size of the union will change the semantics and presumably break things in other ways.

Having said that, glsl-optimizer is only ever ran at build time. And in fact I don't believe this function will ever be run at all with webrender's shaders, as we don't use uniform initializers. So are we able to just ignore this warning?

Comment 3

[Daniel Veditz [:dveditz]]

I'm not sure I understand how the GLfloat helps.

You're right, it doesn't.

Having said that, glsl-optimizer is only ever ran at build time. And in fact I don't believe this function will ever be run at all with webrender's shaders, as we don't use uniform initializers. So are we able to just ignore this warning?

Yeah, if this is build time only we can promise not to abuse ourselves.

I'll unhide this as a security bug, and y'all can decide if you want to throw it in the backlog or wontfix it.