[Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string
gregory hainaut
gregory.hainaut at gmail.com
Fri Apr 7 19:45:10 UTC 2017
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 : "");
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
More information about the mesa-dev
mailing list