<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 01/14/2014 12:30 AM, Paul Berry
wrote:<br>
</div>
<blockquote
cite="mid:CA+yLL65DQDSxvW6uRHM0+TcffTW1LQpf7vbmLp26ZdMxjiSJuA@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div dir="ltr">On 2 January 2014 03:58, Tapani Pälli <span
dir="ltr"><<a moz-do-not-send="true"
href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>></span>
wrote:<br>
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Class
will be used by the shader binary cache implementation.<br>
<br>
Signed-off-by: Tapani Pälli <<a moz-do-not-send="true"
href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>><br>
---<br>
src/glsl/memory_map.h | 174
++++++++++++++++++++++++++++++++++++++++++++++++++<br>
1 file changed, 174 insertions(+)<br>
create mode 100644 src/glsl/memory_map.h<br>
<br>
diff --git a/src/glsl/memory_map.h b/src/glsl/memory_map.h<br>
+ /* read from disk */<br>
+ int map(const char *path)<br>
+ {<br>
+ struct stat stat_info;<br>
+ if (stat(path, &stat_info) != 0)<br>
+ return -1;<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:CA+yLL65DQDSxvW6uRHM0+TcffTW1LQpf7vbmLp26ZdMxjiSJuA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ mode = memory_map::READ_MAP;<br>
+ cache_size = stat_info.st_size;<br>
+<br>
+ fd = open(path, O_RDONLY);<br>
+ if (fd) {<br>
+ cache_mmap_p = cache_mmap = (char *)<br>
+ mmap(NULL, cache_size, PROT_READ,
MAP_PRIVATE, fd, 0);<br>
+ return (cache_mmap == MAP_FAILED) ? -1 : 0;<br>
</blockquote>
<div><br>
</div>
<div>MAP_FAILED is a nonzero value, so if this error
condition ever occurs, the destructor will errneously try
to call munmap().<br>
<br>
What I'd recommend doing instead is:<br>
<br>
</div>
<div>void *mmap_result = mmap(...);<br>
if (mmap_result == MAP_FAILED) {<br>
</div>
<div> close(fd);<br>
</div>
<div> return -1;<br>
</div>
<div>}<br>
</div>
<div>cache_mmap_p = cache_mmap = (char *) mmap_result;<br>
</div>
<div>return 0;<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote
cite="mid:CA+yLL65DQDSxvW6uRHM0+TcffTW1LQpf7vbmLp26ZdMxjiSJuA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+ return -1;<br>
+ }<br>
+<br>
+ /* read from memory */<br>
+ int map(const void *memory, size_t size)<br>
+ {<br>
+ cache_mmap_p = cache_mmap = (char *) memory;<br>
+ cache_size = size;<br>
+ return 0;<br>
+ }<br>
</blockquote>
<div><br>
</div>
<div>IMHO, functions that cannot fail should return void.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
will fix<br>
<br>
<blockquote
cite="mid:CA+yLL65DQDSxvW6uRHM0+TcffTW1LQpf7vbmLp26ZdMxjiSJuA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ /* wrap a portion from another map */<br>
+ int map(memory_map &map, size_t size)<br>
+ {<br>
+ cache_mmap_p = cache_mmap = map.cache_mmap_p;<br>
+ cache_size = size;<br>
+ map.ffwd(size);<br>
+ return 0;<br>
+ }<br>
+<br>
+ inline char *read_string()<br>
+ {<br>
+ char *str = ralloc_strdup(mem_ctx, cache_mmap_p);<br>
+ ffwd(strlen(str)+1);<br>
+ return str;<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
<br>
</div>
<div>My recommendation would be:<br>
<br>
</div>
<div>- Have a boolean in the memory_map class to tell
whether an error has occurred.<br>
</div>
<div>- If we ever try to read beyond the end of the file,
set the boolean and return ralloc_strdup("").<br>
</div>
<div>- At the end of deserialization, check the boolean to
see if an error occurred.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
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?<br>
<br>
<blockquote
cite="mid:CA+yLL65DQDSxvW6uRHM0+TcffTW1LQpf7vbmLp26ZdMxjiSJuA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+/**<br>
+ * read functions per type<br>
+ */<br>
+#define DECL_READER(type) type read_ ##type () {\<br>
+ ffwd(sizeof(type));\<br>
+ return *(type *) (cache_mmap_p - sizeof(type));\<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
Agreed, this is now fixed.<br>
<br>
<blockquote
cite="mid:CA+yLL65DQDSxvW6uRHM0+TcffTW1LQpf7vbmLp26ZdMxjiSJuA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+}<br>
+<br>
+ DECL_READER(int32_t);<br>
+ DECL_READER(int64_t);<br>
+ DECL_READER(uint8_t);<br>
+ DECL_READER(uint32_t);<br>
+<br>
+ inline uint8_t read_bool()<br>
+ {<br>
+ return read_uint8_t();<br>
+ }<br>
+<br>
+ inline void read(void *dst, size_t size)<br>
+ {<br>
+ memcpy(dst, cache_mmap_p, size);<br>
+ ffwd(size);<br>
</blockquote>
<div><br>
</div>
<div>Similar security issue here.<br>
</div>
<div> </div>
</div>
</div>
</div>
</blockquote>
<br>
Fixed also, thanks for pointing these out!<br>
<br>
<blockquote
cite="mid:CA+yLL65DQDSxvW6uRHM0+TcffTW1LQpf7vbmLp26ZdMxjiSJuA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br>
+<br>
+ /* total size of mapped memory */<br>
+ inline int32_t size()<br>
+ {<br>
+ return cache_size;<br>
+ }<br>
+<br>
+private:<br>
+<br>
+ void *mem_ctx;<br>
+<br>
+ /* specifies if we are reading mapped memory or user
passed mem */<br>
+ enum read_mode {<br>
+ READ_MEM = 0,<br>
+ READ_MAP<br>
+ };<br>
+<br>
+ int32_t mode;<br>
+ int32_t fd;<br>
+ int32_t cache_size;<br>
+ char *cache_mmap;<br>
+ char *cache_mmap_p;<br>
+};<br>
+#endif /* ifdef __cplusplus */<br>
+<br>
+#endif /* MEMORY_MAP_H */<br>
<span class="HOEnZb"><font color="#888888">--<br>
1.8.3.1<br>
<br>
</font></span></blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>