On 01/14/2014 12:30 AM, Paul Berry wrote:
On 2 January 2014 03:58, Tapani Pälli <tapani.pa...@intel.com <mailto:tapani.pa...@intel.com>> wrote:

    Class will be used by the shader binary cache implementation.

    Signed-off-by: Tapani Pälli <tapani.pa...@intel.com
    <mailto:tapani.pa...@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



_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to