[Mesa-dev] [PATCH v3] glsl/blob: avoid NULL ptr in blob_write_string/blob_read_string

[Ilia Mirkin]

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.

>
> 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 = &params->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