Issue 4439076: Fix issue 360 - uninit reads in NtCreateThreadEx (Closed)
|
Created:
13 years ago by timurrrr_at_google_com Modified:
13 years ago Reviewers:
bruening Visibility:
Public. |
DescriptionCommited as http://code.google.com/p/drmemory/source/detail?r=273
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address the comments #Patch Set 3 : ... and add a comment #
Total comments: 13
Patch Set 4 : . #
Total comments: 1
Patch Set 5 : handle the post syscall as well #Patch Set 6 : . #
Total comments: 6
Patch Set 7 : . #MessagesTotal messages: 8
Looks like NtCreateThreadEx needs separate handling of one more arg. http://codereview.appspot.com/4439076/diff/1/drmemory/syscall_windows.c File drmemory/syscall_windows.c (right): http://codereview.appspot.com/4439076/diff/1/drmemory/syscall_windows.c#newco... drmemory/syscall_windows.c:913: offsetof(create_thread_info_t, struct_size) + In fact, offsetof() == 0. Should I keep the offsetof() in the code or just remove?
Sign in to reply to this message.
General idea is there but details are off: see comments http://codereview.appspot.com/4439076/diff/1/drmemory/syscall_windows.c File drmemory/syscall_windows.c (right): http://codereview.appspot.com/4439076/diff/1/drmemory/syscall_windows.c#newco... drmemory/syscall_windows.c:914: sizeof(info.struct_size), &info) && this doesn't make sense: for the size you pass offsetof plus sizeof? should instead be adding offsetof to the 1st arg third arg should be &info.struct_size please add a check && between the two safe_reads that info.struct_size <= sizeof(info): let's not have buffer overflows in our own code. maybe turn failure of that into a WARNING log in case the next version of windows increases the size of create_thread_info_t. and if it fails maybe keep going but considering struct_size to be sizeof(info) to gracefully continue.
Sign in to reply to this message.
http://codereview.appspot.com/4439076/diff/1/drmemory/syscall_windows.c File drmemory/syscall_windows.c (right): http://codereview.appspot.com/4439076/diff/1/drmemory/syscall_windows.c#newco... drmemory/syscall_windows.c:914: sizeof(info.struct_size), &info) && On 2011/04/27 14:42:02, bruening wrote: > this doesn't make sense: for the size you pass offsetof plus sizeof? should > instead be adding offsetof to the 1st arg Discussed offline, but still fixed. > third arg should be &info.struct_size > > please add a check && between the two safe_reads that info.struct_size <= > sizeof(info): let's not have buffer overflows in our own code. maybe turn > failure of that into a WARNING log in case the next version of windows increases > the size of create_thread_info_t. and if it fails maybe keep going but > considering struct_size to be sizeof(info) to gracefully continue. Done, good point.
Sign in to reply to this message.
http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c File drmemory/syscall_windows.c (right): http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:912: if (safe_read((byte *)pt->sysarg[10] + offsetof(create_thread_info_t, struct_size), hard to tell in rietveld (another problem of having a code reivew tool that is not in the editor used to write code) but looks too long or if not really close http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:912: if (safe_read((byte *)pt->sysarg[10] + offsetof(create_thread_info_t, struct_size), may be more readable to cast pt->sysarg[10] to create_thread_info_t and then can just do &().struct_size http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:915: DO_ONCE({ LOG(1, "WARNING: create_thread_info_t size larger than known max"); }); line too long (90 max: http://code.google.com/p/dynamorio/wiki/CodeStyle) http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:916: info.struct_size = sizeof(info); /* Avoid overflowing create_thread_info_t */ line too long (90 max: http://code.google.com/p/dynamorio/wiki/CodeStyle) http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:919: /* This is optional, and ommited in i#342 */ why is this comment here? http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:921: info.struct_size, mc, "INFO"); the others are types, in caps only b/c MS defines them that way, so I wouldn't put this in caps if just being descriptive. probably better to use "create_thread_info_t" http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:952: if (safe_read((byte *)pt->sysarg[10], sizeof(info), &info)) { just in case a create_thread_info_t were allocated at bottom of a page, this could fail, so it should only read the size amount as well.
Sign in to reply to this message.
http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c File drmemory/syscall_windows.c (right): http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:912: if (safe_read((byte *)pt->sysarg[10] + offsetof(create_thread_info_t, struct_size), On 2011/04/27 16:07:36, bruening wrote: > may be more readable to cast pt->sysarg[10] to create_thread_info_t and then can > just do &().struct_size Done. http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:915: DO_ONCE({ LOG(1, "WARNING: create_thread_info_t size larger than known max"); }); On 2011/04/27 16:07:36, bruening wrote: > line too long (90 max: http://code.google.com/p/dynamorio/wiki/CodeStyle) Done. http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:916: info.struct_size = sizeof(info); /* Avoid overflowing create_thread_info_t */ On 2011/04/27 16:07:36, bruening wrote: > line too long (90 max: http://code.google.com/p/dynamorio/wiki/CodeStyle) Done. http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:919: /* This is optional, and ommited in i#342 */ Oops, copy-paste error vs bad vim setup vs weird diff when adding tabs On 2011/04/27 16:07:36, bruening wrote: > why is this comment here? http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:921: info.struct_size, mc, "INFO"); On 2011/04/27 16:07:36, bruening wrote: > the others are types, in caps only b/c MS defines them that way, so I wouldn't > put this in caps if just being descriptive. probably better to use > "create_thread_info_t" Done. http://codereview.appspot.com/4439076/diff/6001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:952: if (safe_read((byte *)pt->sysarg[10], sizeof(info), &info)) { mm - so, should I add the same checks here? On 2011/04/27 16:07:36, bruening wrote: > just in case a create_thread_info_t were allocated at bottom of a page, this > could fail, so it should only read the size amount as well.
Sign in to reply to this message.
Looks good. See minor comments. Strange: the pulled-out comments look wrong: should be 3 comments, shows 4 and at wrong places in the summary here. http://codereview.appspot.com/4439076/diff/9001/drmemory/syscall_windows.c File drmemory/syscall_windows.c (right): http://codereview.appspot.com/4439076/diff/9001/drmemory/syscall_windows.c#ne... drmemory/syscall_windows.c:916: info.struct_size = sizeof(info); /* avoid overflowing the sturct */ struct http://codereview.appspot.com/4439076/diff/13001/drmemory/syscall_windows.c#n... drmemory/syscall_windows.c:915: DO_ONCE({ LOG(1, "WARNING: create_thread_info_t size too large"); }); struct http://codereview.appspot.com/4439076/diff/13001/drmemory/syscall_windows.c#n... drmemory/syscall_windows.c:953: check_sysmem(MEMREF_WRITE, sysnum, info.client_id.buffer, don't need this, right, since will have printed warning in pre http://codereview.appspot.com/4439076/diff/13001/drmemory/syscall_windows.c#n... drmemory/syscall_windows.c:954: info.client_id.buffer_size, mc, "PCLIENT_ID"); struct
Sign in to reply to this message.
http://codereview.appspot.com/4439076/diff/13001/drmemory/syscall_windows.c File drmemory/syscall_windows.c (right): http://codereview.appspot.com/4439076/diff/13001/drmemory/syscall_windows.c#n... drmemory/syscall_windows.c:915: info.struct_size = sizeof(info); /* avoid overflowing the sturct */ On 2011/04/27 19:41:41, bruening wrote: > struct Done. http://codereview.appspot.com/4439076/diff/13001/drmemory/syscall_windows.c#n... drmemory/syscall_windows.c:953: DO_ONCE({ LOG(1, "WARNING: create_thread_info_t size too large"); }); On 2011/04/27 19:41:41, bruening wrote: > don't need this, right, since will have printed warning in pre Done. http://codereview.appspot.com/4439076/diff/13001/drmemory/syscall_windows.c#n... drmemory/syscall_windows.c:954: info.struct_size = sizeof(info); /* avoid overflowing the sturct */ On 2011/04/27 19:41:41, bruening wrote: > struct Done.
Sign in to reply to this message.
|
