Re: [Mesa-dev] [PATCH v2] glsl/blob: handle NULL ptr in blob_write_string/blob_read_string

[Timothy Arceri]

On 05/04/17 16:41, gregory hainaut wrote:

On Wed, 5 Apr 2017 09:09:45 +1000
Timothy Arceri  wrote:


On 05/04/17 02:29, Gregory Hainaut wrote:

Context:
Nouveau uses NULL strings for unnamed parameter of texture gather
offsets opcode.

Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau

v2: based on Nicolai feedback


Hi Gregory,

Nicolai suggested you change the caller of create a new helper function
for the case where string can be NULL. e.g blob_write_optional_string()

The change below causes an extra read/write which is not required for
ever other use of this function. Please create the additional function
as a wrapper around blob_write_string()

Thanks,
Tim



Hello Timothy,

I will change it.

However what do you want to do if blob_write_string is called
with a null pointer ? Do you prefer
* Crash
* Assert
* Report an internal error message

I'm leading toward the latter but I'm afraid it might either corrupt
the cache (if we skip the string write or use an empty string). Well we
could just report an error before the crash too.


Feel free to add an assert. It shouldn't happen unless we have a bug.

If we do have a bug the corrupt cache item will be ignored after any 
newer builds of Mesa that include a fix.




Best regards,
Gregory


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] glsl/blob: handle NULL ptr in blob_write_string/blob_read_string

[gregory hainaut]

On Wed, 5 Apr 2017 09:09:45 +1000
Timothy Arceri  wrote:

> On 05/04/17 02:29, Gregory Hainaut wrote:
> > Context:
> > Nouveau uses NULL strings for unnamed parameter of texture gather
> > offsets opcode.
> >
> > Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau
> >
> > v2: based on Nicolai feedback
> 
> Hi Gregory,
> 
> Nicolai suggested you change the caller of create a new helper function 
> for the case where string can be NULL. e.g blob_write_optional_string()
> 
> The change below causes an extra read/write which is not required for 
> ever other use of this function. Please create the additional function 
> as a wrapper around blob_write_string()
> 
> Thanks,
> Tim
> 

Hello Timothy,

I will change it. 

However what do you want to do if blob_write_string is called
with a null pointer ? Do you prefer
* Crash
* Assert
* Report an internal error message

I'm leading toward the latter but I'm afraid it might either corrupt
the cache (if we skip the string write or use an empty string). Well we
could just report an error before the crash too.

Best regards,
Gregory
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] glsl/blob: handle NULL ptr in blob_write_string/blob_read_string

[Timothy Arceri]

On 05/04/17 02:29, Gregory Hainaut wrote:

Context:
Nouveau uses NULL strings for unnamed parameter of texture gather
offsets opcode.

Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau

v2: based on Nicolai feedback


Hi Gregory,

Nicolai suggested you change the caller of create a new helper function 
for the case where string can be NULL. e.g blob_write_optional_string()


The change below causes an extra read/write which is not required for 
ever other use of this function. Please create the additional function 
as a wrapper around blob_write_string()


Thanks,
Tim


Adds an extra flag byte that will be null if the string is null.
This way, null strings are handled transparently for Mesa.

Signed-off-by: Gregory Hainaut 
---
 src/compiler/glsl/blob.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c
index 769ebf1..b520044 100644
--- a/src/compiler/glsl/blob.c
+++ b/src/compiler/glsl/blob.c
@@ -176,7 +176,18 @@ blob_write_intptr(struct blob *blob, intptr_t value)
 bool
 blob_write_string(struct blob *blob, const char *str)
 {
-   return blob_write_bytes(blob, str, strlen(str) + 1);
+   bool ret = true;
+   const uint8_t flag = str != NULL ? 1 : 0;
+
+   ret = blob_write_bytes(blob, , 1);
+
+   if (!ret)
+  return false;
+
+   if (flag)
+  ret = blob_write_bytes(blob, str, strlen(str) + 1);
+
+   return ret;
 }

 void
@@ -293,8 +304,15 @@ blob_read_string(struct blob_reader *blob)
 {
int size;
char *ret;
+   uint8_t *flag;
uint8_t *nul;

+   flag = (uint8_t *)blob_read_bytes(blob, 1);
+
+   if (flag == NULL || *flag == 0) {
+  return NULL;
+   }
+
/* If we're already at the end, then this is an overrun. */
if (blob->current >= blob->end) {
   blob->overrun = true;


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v2] glsl/blob: handle NULL ptr in blob_write_string/blob_read_string

[Gregory Hainaut]

Context:
Nouveau uses NULL strings for unnamed parameter of texture gather
offsets opcode.

Fix piglit crashes of the 'texturegatheroffsets' tests on Nouveau

v2: based on Nicolai feedback
Adds an extra flag byte that will be null if the string is null.
This way, null strings are handled transparently for Mesa.

Signed-off-by: Gregory Hainaut 
---
 src/compiler/glsl/blob.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/compiler/glsl/blob.c b/src/compiler/glsl/blob.c
index 769ebf1..b520044 100644
--- a/src/compiler/glsl/blob.c
+++ b/src/compiler/glsl/blob.c
@@ -176,7 +176,18 @@ blob_write_intptr(struct blob *blob, intptr_t value)
 bool
 blob_write_string(struct blob *blob, const char *str)
 {
-   return blob_write_bytes(blob, str, strlen(str) + 1);
+   bool ret = true;
+   const uint8_t flag = str != NULL ? 1 : 0;
+
+   ret = blob_write_bytes(blob, , 1);
+
+   if (!ret)
+  return false;
+
+   if (flag)
+  ret = blob_write_bytes(blob, str, strlen(str) + 1);
+
+   return ret;
 }
 
 void
@@ -293,8 +304,15 @@ blob_read_string(struct blob_reader *blob)
 {
int size;
char *ret;
+   uint8_t *flag;
uint8_t *nul;
 
+   flag = (uint8_t *)blob_read_bytes(blob, 1);
+
+   if (flag == NULL || *flag == 0) {
+  return NULL;
+   }
+
/* If we're already at the end, then this is an overrun. */
if (blob->current >= blob->end) {
   blob->overrun = true;
-- 
2.1.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev