Re: [Mesa-dev] [wip 3/9] glsl: memory_map helper class for data deserialization
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;
+
Re: [Mesa-dev] [wip 3/9] glsl: memory_map helper class for data deserialization
On 2 January 2014 03:58, Tapani Pälli 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
---
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
new file mode 100644
index 000..1b68b72
--- /dev/null
+++ b/src/glsl/memory_map.h
@@ -0,0 +1,174 @@
+/* -*- c++ -*- */
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
Software),
+ * to deal in the Software without restriction, including without
limitation
+ * the rights to use, copy, modify, merge, publish, distribute,
sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
next
+ * paragraph) shall be included in all copies or substantial portions of
the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#pragma once
+#ifndef MEMORY_MAP_H
+#define MEMORY_MAP_H
+
+#include fcntl.h
+#include unistd.h
+#include sys/mman.h
+#include sys/stat.h
+
+#ifdef __cplusplus
+
+/**
+ * Helper class to read data
+ *
+ * Class can read either from user given memory or from a file. On Linux
+ * file reading wraps around the Posix functions for mapping a file into
+ * the process's address space. Other OS may need different
implementation.
+ */
+class memory_map
+{
+public:
+ memory_map() :
+ mode(memory_map::READ_MEM),
+ fd(0),
+ cache_size(0),
+ cache_mmap(NULL),
+ cache_mmap_p(NULL)
+ {
+ /* only used by read_string() */
+ mem_ctx = ralloc_context(NULL);
+ }
+
+ /* 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.
+
+ 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;
+ }
+ 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.
+
+ /* 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;
+ }
+
+ ~memory_map() {
+ if (cache_mmap mode == READ_MAP) {
+ munmap(cache_mmap, cache_size);
+ close(fd);
+ }
+ ralloc_free(mem_ctx);
+ }
+
+ /* move read pointer forward */
+ inline void ffwd(int len)
+ {
+ cache_mmap_p += len;
+ }
+
+ inline void jump(unsigned pos)
+ {
+ cache_mmap_p = cache_mmap + pos;
+ }
+
+
+ /* position of read pointer */
+ inline uint32_t position()
+ {
+ return cache_mmap_p - cache_mmap;
+ }
+
+ 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
[Mesa-dev] [wip 3/9] glsl: memory_map helper class for data deserialization
Class will be used by the shader binary cache implementation.
Signed-off-by: Tapani Pälli 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
new file mode 100644
index 000..1b68b72
--- /dev/null
+++ b/src/glsl/memory_map.h
@@ -0,0 +1,174 @@
+/* -*- c++ -*- */
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the Software),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#pragma once
+#ifndef MEMORY_MAP_H
+#define MEMORY_MAP_H
+
+#include fcntl.h
+#include unistd.h
+#include sys/mman.h
+#include sys/stat.h
+
+#ifdef __cplusplus
+
+/**
+ * Helper class to read data
+ *
+ * Class can read either from user given memory or from a file. On Linux
+ * file reading wraps around the Posix functions for mapping a file into
+ * the process's address space. Other OS may need different implementation.
+ */
+class memory_map
+{
+public:
+ memory_map() :
+ mode(memory_map::READ_MEM),
+ fd(0),
+ cache_size(0),
+ cache_mmap(NULL),
+ cache_mmap_p(NULL)
+ {
+ /* only used by read_string() */
+ mem_ctx = ralloc_context(NULL);
+ }
+
+ /* read from disk */
+ int map(const char *path)
+ {
+ struct stat stat_info;
+ if (stat(path, stat_info) != 0)
+ return -1;
+
+ 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;
+ }
+ 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;
+ }
+
+ /* 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;
+ }
+
+ ~memory_map() {
+ if (cache_mmap mode == READ_MAP) {
+ munmap(cache_mmap, cache_size);
+ close(fd);
+ }
+ ralloc_free(mem_ctx);
+ }
+
+ /* move read pointer forward */
+ inline void ffwd(int len)
+ {
+ cache_mmap_p += len;
+ }
+
+ inline void jump(unsigned pos)
+ {
+ cache_mmap_p = cache_mmap + pos;
+ }
+
+
+ /* position of read pointer */
+ inline uint32_t position()
+ {
+ return cache_mmap_p - cache_mmap;
+ }
+
+ inline char *read_string()
+ {
+ char *str = ralloc_strdup(mem_ctx, cache_mmap_p);
+ ffwd(strlen(str)+1);
+ return str;
+ }
+
+/**
+ * read functions per type
+ */
+#define DECL_READER(type) type read_ ##type () {\
+ ffwd(sizeof(type));\
+ return *(type *) (cache_mmap_p - sizeof(type));\
+}
+
+ 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);
+ }
+
+ /* 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
