[Mesa-dev] [wip 3/9] glsl: memory_map helper class for data deserialization

[Tapani Pälli]

On 01/14/2014 12:30 AM, Paul Berry wrote:
> On 2 January 2014 03:58, Tapani Pälli <tapani.palli at intel.com 
> <mailto:tapani.palli at intel.com>> wrote:
>
>     Class will be used by the shader binary cache implementation.
>
>     Signed-off-by: Tapani Pälli <tapani.palli at intel.com
>     <mailto:tapani.palli at intel.com>>
>     ---
>      src/glsl/memory_map.h | 174
>     ++++++++++++++++++++++++++++++++++++++++++++++++++
>      1 file changed, 174 insertions(+)
>      create mode 100644 src/glsl/memory_map.h
>
>     diff --git a/src/glsl/memory_map.h b/src/glsl/memory_map.h
>     +   /* read from disk */
>     +   int map(const char *path)
>     +   {
>     +      struct stat stat_info;
>     +      if (stat(path, &stat_info) != 0)
>     +         return -1;
>
>
> As before, I'm not thrilled with the use of -1 to mean failure and 0 
> to mean success, because it forces the caller to use counterintuitive 
> if statements.  I'd prefer for map() to return a bool with true 
> meaning success and false meaning failure.

OK, I will change to use bool. All the 'read from disk' part can be 
actually removed as the extension does not require it. I've been using 
this patch for 2 different sets so it was left unintentionally. It can 
be added if/when Mesa wants to read the files directly itself.

>     +
>     +      mode = memory_map::READ_MAP;
>     +      cache_size = stat_info.st_size;
>     +
>     +      fd = open(path, O_RDONLY);
>     +      if (fd) {
>     +         cache_mmap_p = cache_mmap = (char *)
>     +            mmap(NULL, cache_size, PROT_READ, MAP_PRIVATE, fd, 0);
>     +         return (cache_mmap == MAP_FAILED) ? -1 : 0;
>
>
> MAP_FAILED is a nonzero value, so if this error condition ever occurs, 
> the destructor will errneously try to call munmap().
>
> What I'd recommend doing instead is:
>
> void *mmap_result = mmap(...);
> if (mmap_result == MAP_FAILED) {
>    close(fd);
>    return -1;
> }
> cache_mmap_p = cache_mmap = (char *) mmap_result;
> return 0;

I will fix this to another branch that implements 'automatic cache' and 
maps from disk. It seems I can actually close the fd right away as 
closing it won't unmap the region.

>     +      }
>     +      return -1;
>     +   }
>     +
>     +   /* read from memory */
>     +   int map(const void *memory, size_t size)
>     +   {
>     +      cache_mmap_p = cache_mmap = (char *) memory;
>     +      cache_size = size;
>     +      return 0;
>     +   }
>
>
> IMHO, functions that cannot fail should return void.

will fix

>     +
>     +   /* wrap a portion from another map */
>     +   int map(memory_map &map, size_t size)
>     +   {
>     +      cache_mmap_p = cache_mmap = map.cache_mmap_p;
>     +      cache_size = size;
>     +      map.ffwd(size);
>     +      return 0;
>     +   }
>     +
>     +   inline char *read_string()
>     +   {
>     +      char *str = ralloc_strdup(mem_ctx, cache_mmap_p);
>     +      ffwd(strlen(str)+1);
>     +      return str;
>
>
> This is problematic from a security perspective.  If the client 
> provides corrupted data that ends in a truncated string (lacking a 
> null terminator) that could cause ralloc_strdup() to try to read 
> beyond the end of the file.  We need to make sure the code doesn't try 
> to read beyond the end of file, even if the data is corrupted or 
> truncated.
>
> My recommendation would be:
>
> - Have a boolean in the memory_map class to tell whether an error has 
> occurred.
> - If we ever try to read beyond the end of the file, set the boolean 
> and return ralloc_strdup("").
> - At the end of deserialization, check the boolean to see if an error 
> occurred.

I've done this now to other reads, but read_string is problematic. I 
think safest approach would be that writer actually writes the length 
there so that reader does not need to make guesses how long string could 
be and if it is terminated or not?

>     +   }
>     +
>     +/**
>     + * read functions per type
>     + */
>     +#define DECL_READER(type) type read_ ##type () {\
>     +   ffwd(sizeof(type));\
>     +   return *(type *) (cache_mmap_p - sizeof(type));\
>
>
> This is a similar security issue if the input is truncated.  If 
> cache_mmap_p points beyond the end of the file after the call to 
> ffwd(), we should set the error boolean and return 0.

Agreed, this is now fixed.

>     +}
>     +
>     +   DECL_READER(int32_t);
>     +   DECL_READER(int64_t);
>     +   DECL_READER(uint8_t);
>     +   DECL_READER(uint32_t);
>     +
>     +   inline uint8_t read_bool()
>     +   {
>     +      return read_uint8_t();
>     +   }
>     +
>     +   inline void read(void *dst, size_t size)
>     +   {
>     +      memcpy(dst, cache_mmap_p, size);
>     +      ffwd(size);
>
>
> Similar security issue here.

Fixed also, thanks for pointing these out!

>     +   }
>     +
>     +   /* total size of mapped memory */
>     +   inline int32_t size()
>     +   {
>     +      return cache_size;
>     +   }
>     +
>     +private:
>     +
>     +   void *mem_ctx;
>     +
>     +   /* specifies if we are reading mapped memory or user passed mem */
>     +   enum read_mode {
>     +      READ_MEM = 0,
>     +      READ_MAP
>     +   };
>     +
>     +   int32_t mode;
>     +   int32_t fd;
>     +   int32_t cache_size;
>     +   char *cache_mmap;
>     +   char *cache_mmap_p;
>     +};
>     +#endif /* ifdef __cplusplus */
>     +
>     +#endif /* MEMORY_MAP_H */
>     --
>     1.8.3.1
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140114/697e75bb/attachment-0001.html>