[Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string
Nicolai Hähnle
nhaehnle at gmail.com
Tue Apr 11 09:59:17 UTC 2017
On 07.04.2017 21:45, gregory hainaut wrote:
> On Thu, 6 Apr 2017 00:21:19 +0200
> gregory hainaut <gregory.hainaut at gmail.com> wrote:
>
>> On Wed, 5 Apr 2017 14:22:00 -0400
>> Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>
>>> On Wed, Apr 5, 2017 at 1:12 PM, Gregory Hainaut
>>> <gregory.hainaut at gmail.com> wrote:
>>>> Context:
>>>> Nouveau uses NULL strings for unnamed parameter of texture gather
>>>> offsets opcode.
>>>
>>> To be clear, this isn't a "nouveau" thing, as it is well downstream of
>>> all this GLSL stuff. And FWIW llvmpipe/softpipe also support the
>>> 4-offset TG4 (I think). It's a GLSL IR thing. If it needs changing,
>>> you can change how GLSL IR represents this stuff, instead of
>>> necessarily supporting this wart.
>>>
>>> Unfortunately I don't understand the problem clearly enough to tell
>>> what's going on and what the proper solution is.
>>>
>>
>> Ilia,
>>
>> Yes you're right it isn't limited to Nouveau. And actually it could appear
>> in future glsl code. Actually there is another call to _mesa_add_parameter
>> in st_init_atifs_prog from state_tracker/st_atifs_to_tgsi.c
>>
>> If I try to do a summary.
>>
>> The problem is "what is the expected name of an unnamed parameter ?"
>> The 2 possibles answer are NULL or emptry (AKA "").
>>
>> Nicolai raise the valid point that code could handle NULL and empty differently.
>>
>> I tried to do various grep without success. I don't think there is any difference
>> between NULL and "" but easy to say hard to demonstrate.
>>
>> However I don't think that allowing NULL pointer in name is a good idea.
>>
>> For example st_nir_lookup_parameter_index (st_glsl_to_nir.cpp) use
>> this kind of code. I'm afraid that strncmp with a NULL pointer is an
>> undefined behavior.
>>
>> for (unsigned i = 0; i < params->NumParameters; i++) {
>> struct gl_program_parameter *p = ¶ms->Parameters[i];
>> if ((strncmp(p->Name, name, namelen) == 0) &&
>>
>>
>> The YOLO solution is appealing. We change _mesa_add_parameter NULL name
>> parameter to "". Then we run piglit.
>>
>>
>> Best regards,
>> Gregory
>
> Hello All,
>
> I commented the "Name" field in the struct to see the gcc error message.
> I did also some grep to be sure I didn't miss anything.
>
> I didn't find anything that differentiate a NULL pointer from an empty
> string. Actually "Name" is used inside printf and to lookup the index/location.
> Note: some code doesn't check for NULL pointer.
>
> copy_indirect_accessed_array (prog_parameter_layout.c) will use NULL on
> purpose to avoid a double-free (due to pointer memcpy). But it is only
> a temporary struct that will be freed by the caller (_mesa_layout_parameters)
>
> Conclusion, it would be enough to add a parameter with
> p->Name = strdup(name ? name : "");
I like this variant. It eliminates the need for special NULL-handling
everywhere.
Thanks for doing those investigations!
Nicolai
>
> Or we can keep the *string_optional function. It is up to you. Tell me
> what solution do you prefer and I will redo the patch accordingly.
>
> Best regards,
> Gregory
>
>
>>>>
>>>> Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau
>>>>
>>>> v3: Redo patch based on Nicolai/Timothy feedback
>>>> Create dedicated blob_write_string_optional/blob_read_string_optional to
>>>> handle null pointer string.
>>>> Add an assert in blob_write_string to ease debug
>>>>
>>>> Signed-off-by: Gregory Hainaut <gregory.hainaut at gmail.com>
>>>> ---
>>>> src/compiler/glsl/blob.c | 33 +++++++++++++++++++++++++++++++++
>>>> src/compiler/glsl/blob.h | 27 +++++++++++++++++++++++++++
>>>> src/compiler/glsl/shader_cache.cpp | 4 ++--
>>>> 3 files changed, 62 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c
>>>> index 769ebf1..62d6bf5 100644
>>>> --- a/src/compiler/glsl/blob.c
>>>> +++ b/src/compiler/glsl/blob.c
>>>> @@ -176,9 +176,29 @@ blob_write_intptr(struct blob *blob, intptr_t value)
>>>> bool
>>>> blob_write_string(struct blob *blob, const char *str)
>>>> {
>>>> + // Please use blob_write_string_optional instead
>>>> + assert(str != NULL);
>>>> +
>>>> return blob_write_bytes(blob, str, strlen(str) + 1);
>>>> }
>>>>
>>>> +bool
>>>> +blob_write_string_optional(struct blob *blob, const char *str)
>>>> +{
>>>> + bool ret;
>>>> + const uint8_t flag = str != NULL ? 1 : 0;
>>>> +
>>>> + ret = blob_write_bytes(blob, &flag, 1);
>>>> +
>>>> + if (!ret)
>>>> + return false;
>>>> +
>>>> + if (str != NULL)
>>>> + ret = blob_write_string(blob, str);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> void
>>>> blob_reader_init(struct blob_reader *blob, uint8_t *data, size_t size)
>>>> {
>>>> @@ -321,3 +341,16 @@ blob_read_string(struct blob_reader *blob)
>>>>
>>>> return ret;
>>>> }
>>>> +
>>>> +char *
>>>> +blob_read_string_optional(struct blob_reader *blob)
>>>> +{
>>>> + uint8_t *flag;
>>>> + flag = (uint8_t *)blob_read_bytes(blob, 1);
>>>> +
>>>> + if (flag == NULL || *flag == 0) {
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + return blob_read_string(blob);
>>>> +}
>>>> diff --git a/src/compiler/glsl/blob.h b/src/compiler/glsl/blob.h
>>>> index 940c81e..2f58cc3 100644
>>>> --- a/src/compiler/glsl/blob.h
>>>> +++ b/src/compiler/glsl/blob.h
>>>> @@ -203,6 +203,7 @@ blob_write_intptr(struct blob *blob, intptr_t value);
>>>>
>>>> /**
>>>> * Add a NULL-terminated string to a blob, (including the NULL terminator).
>>>> + * If str could be NULL, blob_write_string_optional must be used instead.
>>>> *
>>>> * \return True unless allocation failed.
>>>> */
>>>> @@ -210,6 +211,15 @@ bool
>>>> blob_write_string(struct blob *blob, const char *str);
>>>>
>>>> /**
>>>> + * Add a flag + a NULL-terminated string to a blob, (including the NULL
>>>> + * terminator). The flag will be zero if str is NULL.
>>>> + *
>>>> + * \return True unless allocation failed.
>>>> + */
>>>> +bool
>>>> +blob_write_string_optional(struct blob *blob, const char *str);
>>>> +
>>>> +/**
>>>> * Start reading a blob, (initializing the contents of \blob for reading).
>>>> *
>>>> * After this call, the caller can use the various blob_read_* functions to
>>>> @@ -294,6 +304,23 @@ blob_read_intptr(struct blob_reader *blob);
>>>> char *
>>>> blob_read_string(struct blob_reader *blob);
>>>>
>>>> +/**
>>>> + * Read a NULL-terminated string from the current location, (and update the
>>>> + * current location to just past this string).
>>>> + *
>>>> + * \note Similar as blob_read_string but return NULL if written string was NULL.
>>>> + *
>>>> + * \note The memory returned belongs to the data underlying the blob reader. The
>>>> + * caller must copy the string in order to use the string after the lifetime
>>>> + * of the data underlying the blob reader.
>>>> + *
>>>> + * \return The string read (see note above about memory lifetime). However, if
>>>> + * there is no NULL byte remaining within the blob, this function returns
>>>> + * NULL.
>>>> + */
>>>> +char *
>>>> +blob_read_string_optional(struct blob_reader *blob);
>>>> +
>>>> #ifdef __cplusplus
>>>> }
>>>> #endif
>>>> diff --git a/src/compiler/glsl/shader_cache.cpp b/src/compiler/glsl/shader_cache.cpp
>>>> index f5e6a22..e07d2c4 100644
>>>> --- a/src/compiler/glsl/shader_cache.cpp
>>>> +++ b/src/compiler/glsl/shader_cache.cpp
>>>> @@ -1078,7 +1078,7 @@ write_shader_parameters(struct blob *metadata,
>>>> struct gl_program_parameter *param = ¶ms->Parameters[i];
>>>>
>>>> blob_write_uint32(metadata, param->Type);
>>>> - blob_write_string(metadata, param->Name);
>>>> + blob_write_string_optional(metadata, param->Name);
>>>> blob_write_uint32(metadata, param->Size);
>>>> blob_write_uint32(metadata, param->DataType);
>>>> blob_write_bytes(metadata, param->StateIndexes,
>>>> @@ -1104,7 +1104,7 @@ read_shader_parameters(struct blob_reader *metadata,
>>>> _mesa_reserve_parameter_storage(params, num_parameters);
>>>> while (i < num_parameters) {
>>>> gl_register_file type = (gl_register_file) blob_read_uint32(metadata);
>>>> - const char *name = blob_read_string(metadata);
>>>> + const char *name = blob_read_string_optional(metadata);
>>>> unsigned size = blob_read_uint32(metadata);
>>>> unsigned data_type = blob_read_uint32(metadata);
>>>> blob_copy_bytes(metadata, (uint8_t *) state_indexes,
>>>> --
>>>> 2.1.4
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
More information about the mesa-dev
mailing list