[Mesa-dev] [PATCH] mesa: Move transform feedback error check to reduce array overflow risk.

[Paul Berry]

On 9 January 2012 13:00, Ian Romanick <idr at freedesktop.org> wrote:

> On 01/09/2012 12:02 PM, Paul Berry wrote:
>
>> Previous to this patch, we didn't do the limit check for
>> MAX_TRANSFORM_FEEDBACK_**INTERLEAVED_COMPONENTS until the end of the
>> store_tfeedback_info() function, *after* storing all of the transform
>> feedback info in the gl_transform_feedback_info::**Outputs array.  This
>> meant that the limit check wouldn't prevent us from overflowing the
>> array and corrupting memory.
>>
>> This patch moves the limit check to the top of tfeedback_decl::store()
>> so that there is no risk of overflowing the array.  It also adds
>> assertions to verify that the checks for
>> MAX_TRANSFORM_FEEDBACK_**INTERLEAVED_COMPONENTS and
>> MAX_TRANSFORM_FEEDBACK_**SEPARATE_COMPONENTS are sufficient to avoid
>> array overflow.
>>
>> Note: strictly speaking this patch isn't necessary, since the maximum
>> possible number of varyings is MAX_VARYING (16), whereas the size of
>> the Outputs array is MAX_PROGRAM_OUTPUTS (64), so it's impossible to
>> have enough varyings to overflow the array.  However it seems prudent
>>
>
> Why is Outputs so large?
>

According to the spec, MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS must
be at least 64.  In the worst case, one element of the Outputs array will
be used up per component, so the array size needs to be at least 64.


>
>  to do the limit check before the array access in case these limits
>> change in the future.
>> ---
>>  src/glsl/linker.cpp |   52 ++++++++++++++++++++++++++++++**
>> +-------------------
>>  1 files changed, 32 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 39a9c46..0d85aee 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -1388,7 +1388,7 @@ public:
>>     static bool is_same(const tfeedback_decl&x, const tfeedback_decl&y);
>>     bool assign_location(struct gl_context *ctx, struct gl_shader_program
>> *prog,
>>                          ir_variable *output_var);
>> -   bool store(struct gl_shader_program *prog,
>> +   bool store(struct gl_context *ctx, struct gl_shader_program *prog,
>>                struct gl_transform_feedback_info *info, unsigned buffer,
>>              unsigned varying) const;
>>
>> @@ -1631,7 +1631,7 @@ tfeedback_decl::assign_**location(struct
>> gl_context *ctx,
>>   * is returned.
>>   */
>>  bool
>> -tfeedback_decl::store(struct gl_shader_program *prog,
>> +tfeedback_decl::store(struct gl_context *ctx, struct gl_shader_program
>> *prog,
>>                        struct gl_transform_feedback_info *info,
>>                        unsigned buffer, unsigned varying) const
>>  {
>> @@ -1647,6 +1647,35 @@ tfeedback_decl::store(struct gl_shader_program
>> *prog,
>>                     this->orig_name);
>>        return false;
>>     }
>> +
>> +   /* From GL_EXT_transform_feedback:
>> +    *   A program will fail to link if:
>> +    *
>> +    *     * the total number of components to capture is greater than
>> +    *       the constant MAX_TRANSFORM_FEEDBACK_**
>> INTERLEAVED_COMPONENTS_EXT
>> +    *       and the buffer mode is INTERLEAVED_ATTRIBS_EXT.
>> +    */
>> +   if (prog->TransformFeedback.**BufferMode == GL_INTERLEAVED_ATTRIBS&&
>> +       info->BufferStride[buffer] + this->num_components()>
>> +       ctx->Const.**MaxTransformFeedbackInterleave**dComponents) {
>> +      linker_error(prog, "The MAX_TRANSFORM_FEEDBACK_**INTERLEAVED_COMPONENTS
>> "
>> +                   "limit has been exceeded.");
>> +      return false;
>> +   }
>> +
>> +   /* Verify that the checks on MAX_TRANSFORM_FEEDBACK_**
>> INTERLEAVED_COMPONENTS
>> +    * and MAX_TRANSFORM_FEEDBACK_**SEPARATE_COMPONENTS are sufficient
>> to prevent
>> +    * overflow of info->Outputs[].  In worst case we generate one entry
>> in
>> +    * Outputs[] per component so a conservative check is to verify that
>> the
>> +    * size of the array is greater than or equal to both
>> +    * MAX_TRANSFORM_FEEDBACK_**INTERLEAVED_COMPONENTS and
>> +    * MAX_TRANSFORM_FEEDBACK_**SEPARATE_COMPONENTS.
>> +    */
>> +   assert(Elements(info->Outputs)**>=
>> +          ctx->Const.**MaxTransformFeedbackInterleave**dComponents);
>> +   assert(Elements(info->Outputs)**>=
>> +          ctx->Const.**MaxTransformFeedbackSeparateCo**mponents);
>> +
>>     unsigned translated_size = this->size;
>>     if (this->is_clip_distance_mesa)
>>        translated_size = (translated_size + 3) / 4;
>> @@ -1943,7 +1972,6 @@ store_tfeedback_info(struct gl_context *ctx, struct
>> gl_shader_program *prog,
>>                       unsigned num_tfeedback_decls,
>>                       tfeedback_decl *tfeedback_decls)
>>  {
>> -   unsigned total_tfeedback_components = 0;
>>     bool separate_attribs_mode =
>>        prog->TransformFeedback.**BufferMode == GL_SEPARATE_ATTRIBS;
>>
>> @@ -1962,25 +1990,9 @@ store_tfeedback_info(struct gl_context *ctx,
>> struct gl_shader_program *prog,
>>
>>     for (unsigned i = 0; i<  num_tfeedback_decls; ++i) {
>>        unsigned buffer = separate_attribs_mode ? i : 0;
>> -      if (!tfeedback_decls[i].store(**prog,&prog->**
>> LinkedTransformFeedback,
>> +      if (!tfeedback_decls[i].store(**ctx, prog,&prog->**
>> LinkedTransformFeedback,
>>
>>                                      buffer, i))
>>           return false;
>> -      total_tfeedback_components += tfeedback_decls[i].num_**
>> components();
>> -   }
>> -
>> -   /* From GL_EXT_transform_feedback:
>> -    *   A program will fail to link if:
>> -    *
>> -    *     * the total number of components to capture is greater than
>> -    *       the constant MAX_TRANSFORM_FEEDBACK_**
>> INTERLEAVED_COMPONENTS_EXT
>> -    *       and the buffer mode is INTERLEAVED_ATTRIBS_EXT.
>> -    */
>> -   if (prog->TransformFeedback.**BufferMode == GL_INTERLEAVED_ATTRIBS&&
>> -       total_tfeedback_components>
>> -       ctx->Const.**MaxTransformFeedbackInterleave**dComponents) {
>> -      linker_error(prog, "The MAX_TRANSFORM_FEEDBACK_**INTERLEAVED_COMPONENTS
>> "
>> -                   "limit has been exceeded.");
>> -      return false;
>>     }
>>
>>     return true;
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120109/b455e9e0/attachment-0001.htm>