Issue 257120043: i#1734 Create Dr. Fuzz: add exception API
|
Created:
8 years, 8 months ago by Byron Modified:
8 years, 8 months ago CC:
drmemory-devs_googlegroups.com Visibility:
Public. |
DescriptionCommit log for first patchset:
---------------
i#1734 Create Dr. Fuzz: add exception API
This CL adds the exception API, along with a
general iterator for walking the set of fuzz
targets currently live on the call stack.
---------------
Patch Set 1 #
Total comments: 22
Patch Set 2 : PTAL #
Total comments: 38
Patch Set 3 : PTAL #
Total comments: 13
Patch Set 4 : PTAL #
Total comments: 12
Patch Set 5 : PTAL #
Total comments: 31
Patch Set 6 : bruening, PTAL #
Total comments: 10
Patch Set 7 : PTAL #
Total comments: 39
Patch Set 8 : Committed #
MessagesTotal messages: 31
This code has been tested in the current drfuzz test client. It correctly reports the fuzz targets that were being executed when a fatal segfault occurs. The test is omitted for brevity.
Sign in to reply to this message.
The main thing is I am not sure if the thread_crash callback should be registered via an option or via a register function. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.c#newcode529 drfuzz/drfuzz.c:529: e->crash_id = crash->sig; nit, it is not really id, maybe crash_code. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.c#newcode572 drfuzz/drfuzz.c:572: return ((crash)->sig == SIGSEGV); nit, add comment about adding other possible signals like SIGBUS, SIGABRT). https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.c#newcode614 drfuzz/drfuzz.c:614: create_target_iterator(void *dcontext, fuzz_pass_context_t *fp) nit, can we move this function above drfuzz_target_iterator_start and close to drfuzz_target_iterator_free/stop, it is easier to compare the create and free. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode64 drfuzz/drfuzz.h:64: * the beginning of each fuzz iteration. beginning of each fuzz iteration? are we iterating all and only all live targets, if yes, we should be access while the iteration, right? https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode76 drfuzz/drfuzz.h:76: typedef struct _drfuzz_target_iterator_t drfuzz_target_iterator_t; If we are not exposing the target_iterator data structure, we can just do something like typedef void * drfuzz_target_iterator_t; and in drfuzz.c, we just use target_iterator_t, xref dr_module_iterator_t in dynamorio/core/lib/instrument.h https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode86 drfuzz/drfuzz.h:86: thread_id_t thread_id; do you think if we should add "void *user_data" to allow client to add their own data for each exception, e.g., callstack on the exception. In that case, we also need a callback to allow drfuzz to call to free the user_data. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode102 drfuzz/drfuzz.h:102: drfuzz_target_iterator_t *targets); Why do we want to use options to add the callback? If that's the only thing we gonna add to the drfuzz_option, maybe we should add drfuzz_register_thread_terminate_callback() for thread exit without complete fuzzing? on an exception, tools like Dr.Memory probably want to construct a callstack, which requires mc, maybe we want to add drfuzz_register_exception_callback, or just use drmgr's register_exception_callback. And the callback if we want to remove the user_data in exception. I guess it should be in a separate CL. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode233 drfuzz/drfuzz.h:233: * frames, and use drfuzz_target_iterator_free() to free the iterator and all frames. nit, typically, we use iterator_stop() to destroy the iterator. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode243 drfuzz/drfuzz.h:243: drfuzz_target_iterator_next(drfuzz_target_iterator_t *iter); do we want to have _hasnext, probably not necessary. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode250 drfuzz/drfuzz.h:250: drfuzz_target_iterator_free(drfuzz_target_iterator_t *iter); nit,_stop https://codereview.appspot.com/257120043/diff/1/tests/framework/drfuzz_client... File tests/framework/drfuzz_client_empty.c (right): https://codereview.appspot.com/257120043/diff/1/tests/framework/drfuzz_client... tests/framework/drfuzz_client_empty.c:39: static drfuzz_options_t drfuzz_ops; why does it have to be a static, cannot it be local variable on stack?
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.c#newcode529 drfuzz/drfuzz.c:529: e->crash_id = crash->sig; On 2015/07/31 20:03:03, zhaoqin wrote: > nit, it is not really id, maybe crash_code. Done. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.c#newcode572 drfuzz/drfuzz.c:572: return ((crash)->sig == SIGSEGV); On 2015/07/31 20:03:04, zhaoqin wrote: > nit, add comment about adding other possible signals like SIGBUS, SIGABRT). That's the XXX above. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.c#newcode614 drfuzz/drfuzz.c:614: create_target_iterator(void *dcontext, fuzz_pass_context_t *fp) On 2015/07/31 20:03:03, zhaoqin wrote: > nit, can we move this function above drfuzz_target_iterator_start and close to > drfuzz_target_iterator_free/stop, it is easier to compare the create and free. Done. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode64 drfuzz/drfuzz.h:64: * the beginning of each fuzz iteration. On 2015/07/31 20:03:04, zhaoqin wrote: > beginning of each fuzz iteration? > are we iterating all and only all live targets, if yes, we should be access > while the iteration, right? Not quite sure what you mean. Yes, the user can access the values during fuzzing, but the purpose of this struct is to save the values for access after a crash. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode76 drfuzz/drfuzz.h:76: typedef struct _drfuzz_target_iterator_t drfuzz_target_iterator_t; On 2015/07/31 20:03:04, zhaoqin wrote: > If we are not exposing the target_iterator data structure, we can just do > something like > typedef void * drfuzz_target_iterator_t; > and in drfuzz.c, we just use target_iterator_t, > xref dr_module_iterator_t in dynamorio/core/lib/instrument.h Done. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode86 drfuzz/drfuzz.h:86: thread_id_t thread_id; On 2015/07/31 20:03:04, zhaoqin wrote: > do you think if we should add "void *user_data" to allow client to add their own > data for each exception, e.g., callstack on the exception. > > In that case, we also need a callback to allow drfuzz to call to free the > user_data. This would also require a separate callback per exception--currently we wait until the thread actually crashes out before making any callbacks. It seems ok to me if you think people will use it. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode102 drfuzz/drfuzz.h:102: drfuzz_target_iterator_t *targets); On 2015/07/31 20:03:04, zhaoqin wrote: > Why do we want to use options to add the callback? > If that's the only thing we gonna add to the drfuzz_option, maybe we should add > drfuzz_register_thread_terminate_callback() for thread exit without complete > fuzzing? > > > on an exception, tools like Dr.Memory probably want to construct a callstack, > which requires mc, maybe we want to add drfuzz_register_exception_callback, > or just use drmgr's register_exception_callback. > And the callback if we want to remove the user_data in exception. > I guess it should be in a separate CL. > ok, let's add the exception callback with user_data in another CL. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode233 drfuzz/drfuzz.h:233: * frames, and use drfuzz_target_iterator_free() to free the iterator and all frames. On 2015/07/31 20:03:04, zhaoqin wrote: > nit, typically, we use iterator_stop() to destroy the iterator. Done. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode243 drfuzz/drfuzz.h:243: drfuzz_target_iterator_next(drfuzz_target_iterator_t *iter); On 2015/07/31 20:03:04, zhaoqin wrote: > do we want to have _hasnext, probably not necessary. It returns NULL after the last frame, so the user knows to stop. https://codereview.appspot.com/257120043/diff/1/drfuzz/drfuzz.h#newcode250 drfuzz/drfuzz.h:250: drfuzz_target_iterator_free(drfuzz_target_iterator_t *iter); On 2015/07/31 20:03:04, zhaoqin wrote: > nit,_stop Done. https://codereview.appspot.com/257120043/diff/1/tests/framework/drfuzz_client... File tests/framework/drfuzz_client_empty.c (right): https://codereview.appspot.com/257120043/diff/1/tests/framework/drfuzz_client... tests/framework/drfuzz_client_empty.c:39: static drfuzz_options_t drfuzz_ops; On 2015/07/31 20:03:04, zhaoqin wrote: > why does it have to be a static, cannot it be local variable on stack? Removed in favor of callbacks. https://codereview.appspot.com/257120043/diff/60001/common/utils.h File common/utils.h (right): https://codereview.appspot.com/257120043/diff/60001/common/utils.h#newcode814 common/utils.h:814: * CALLBACK LIST This whole thing could be a separate CL, though it's fairly trivial. https://codereview.appspot.com/257120043/diff/60001/make/git/git_review.sh File make/git/git_review.sh (right): https://codereview.appspot.com/257120043/diff/60001/make/git/git_review.sh#ne... make/git/git_review.sh:123: -t "${subject}" -m "${msg}" ${email} HEAD^1) I'll revert this change before committing, but I couldn't get it to take the right set of files any other way. After rebasing master, this script always does the wrong thing.
Sign in to reply to this message.
https://codereview.appspot.com/257120043/diff/60001/make/git/git_review.sh File make/git/git_review.sh (right): https://codereview.appspot.com/257120043/diff/60001/make/git/git_review.sh#ne... make/git/git_review.sh:123: -t "${subject}" -m "${msg}" ${email} HEAD^1) On 2015/08/04 17:18:55, Byron wrote: > I'll revert this change before committing, but I couldn't get it to take the > right set of files any other way. After rebasing master, this script always does > the wrong thing. Do you mean "before rebasing"? You could also the alias to point at a script outside of version control to avoid something like this corrupting the diff.
Sign in to reply to this message.
one of my concern is the exception callback. https://codereview.appspot.com/257120043/diff/60001/common/utils.h File common/utils.h (right): https://codereview.appspot.com/257120043/diff/60001/common/utils.h#newcode814 common/utils.h:814: * CALLBACK LIST On 2015/08/04 17:18:55, Byron wrote: > This whole thing could be a separate CL, though it's fairly trivial. I was thinking about only support one callback, so a single pointer would do the work, and maybe we can add support for multiple ones later, but I guess we can add them now. Then do we need worry about the ordering, or the order is the reverse order of the calling? https://codereview.appspot.com/257120043/diff/60001/common/utils.h#newcode824 common/utils.h:824: callback_list_delete(callback_list_t *list); destroy? you are destroy the whole list, right? https://codereview.appspot.com/257120043/diff/60001/common/utils.h#newcode839 common/utils.h:839: callback_list_iterator_end(callback_list_iterator_t *iter); stop, I like to pair the start with stop. https://codereview.appspot.com/257120043/diff/60001/common/utils_shared.c File common/utils_shared.c (right): https://codereview.appspot.com/257120043/diff/60001/common/utils_shared.c#new... common/utils_shared.c:118: global_free(list, sizeof(callback_list_t), list->type); so you assume the list is empty? Does it make sense to walk the list and free each single one here, to avoid any possible leaks. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c#newcode147 drfuzz/drfuzz.c:147: typedef void (*crash_callback)(uint exception_count, callback_t, as this is a type. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c#newcode150 drfuzz/drfuzz.c:150: drfuzz_target_iterator_t *targets); should this be in drfuzz.h, so client can use it? https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c#newcode589 drfuzz/drfuzz.c:589: sizeof(reg_t) * iter->targets[i].arg_count, HEAPSTAT_MISC); nit, we often use things like sizeof(iter->targets[i].arg_values[0]) to get the size, and it works even after we change the type of the arg_values. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c#newcode626 drfuzz/drfuzz.c:626: capture_crash(dcontext, &fp->current_exception_chain->first, crash); so on the first exception, the ->last is left there? should it be last be also initialized it too. Hmm, then there might be complication of two pointer pointing to the same struct, should avoid double free. Is it possible just remember the last? I think it would be simpler. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode189 drfuzz/drfuzz.h:189: * Register a crash notification callback, which is called by drfuzz when an exception I feel that maybe early thread terminate callback is a better name, as we call this callback if the fuzz chain is not NULL, i.e., early terminate, not even necessary caused by the exception. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode192 drfuzz/drfuzz.h:192: * drfuzz_target_iterator_next(), but do not free the iterator (it has been pre-started). you mean the caller will call the _stop instead? https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode193 drfuzz/drfuzz.h:193: */ we also want to say if we want to support multiple callbacks, and what's the order if there are more than one. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode197 drfuzz/drfuzz.h:197: drfuzz_exception_t *last_exception, do you think if we want these two args. From the implementation, it looks like we will have an uninit last_exception value, at least it should be NULL if one exception happens, then which one should be NULL, first or last? Maybe just keep last? https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode240 drfuzz/drfuzz.h:240: * frames, and use drfuzz_target_iterator_free() to free the iterator and all frames. _stop. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode249 drfuzz/drfuzz.h:249: drfuzz_target_frame_t * typically we return DRMF_STATUS_T, kind of strange we change only this one, but it does not make much sense to return DRMF_STATUS_T here either. https://codereview.appspot.com/257120043/diff/60001/make/git/git_review.sh File make/git/git_review.sh (right): https://codereview.appspot.com/257120043/diff/60001/make/git/git_review.sh#ne... make/git/git_review.sh:123: -t "${subject}" -m "${msg}" ${email} HEAD^1) I think I changed that already. https://codereview.appspot.com/257120043/diff/60001/tests/framework/drfuzz_cl... File tests/framework/drfuzz_client_empty.c (right): https://codereview.appspot.com/257120043/diff/60001/tests/framework/drfuzz_cl... tests/framework/drfuzz_client_empty.c:52: ?
Sign in to reply to this message.
This new feature needs a test. Should I add one here or in a later CL? https://codereview.appspot.com/257120043/diff/60001/common/utils.h File common/utils.h (right): https://codereview.appspot.com/257120043/diff/60001/common/utils.h#newcode814 common/utils.h:814: * CALLBACK LIST On 2015/08/04 20:14:13, zhaoqin wrote: > On 2015/08/04 17:18:55, Byron wrote: > > This whole thing could be a separate CL, though it's fairly trivial. > > I was thinking about only support one callback, so a single pointer would do the > work, and maybe we can add support for multiple ones later, but I guess we can > add them now. > > Then do we need worry about the ordering, or the order is the reverse order of > the calling? I'd say let's add priorities later, if we need them. https://codereview.appspot.com/257120043/diff/60001/common/utils.h#newcode824 common/utils.h:824: callback_list_delete(callback_list_t *list); On 2015/08/04 20:14:13, zhaoqin wrote: > destroy? you are destroy the whole list, right? Added a comment. https://codereview.appspot.com/257120043/diff/60001/common/utils.h#newcode839 common/utils.h:839: callback_list_iterator_end(callback_list_iterator_t *iter); On 2015/08/04 20:14:13, zhaoqin wrote: > stop, I like to pair the start with stop. Done. https://codereview.appspot.com/257120043/diff/60001/common/utils_shared.c File common/utils_shared.c (right): https://codereview.appspot.com/257120043/diff/60001/common/utils_shared.c#new... common/utils_shared.c:118: global_free(list, sizeof(callback_list_t), list->type); On 2015/08/04 20:14:13, zhaoqin wrote: > so you assume the list is empty? > Does it make sense to walk the list and free each single one here, to avoid any > possible leaks. It's a bug. Fixed. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c#newcode147 drfuzz/drfuzz.c:147: typedef void (*crash_callback)(uint exception_count, On 2015/08/04 20:14:13, zhaoqin wrote: > callback_t, as this is a type. Done. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c#newcode150 drfuzz/drfuzz.c:150: drfuzz_target_iterator_t *targets); On 2015/08/04 20:14:13, zhaoqin wrote: > should this be in drfuzz.h, so client can use it? Derek suggested to keep it private because that makes it easier to change the callback signature later on. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c#newcode589 drfuzz/drfuzz.c:589: sizeof(reg_t) * iter->targets[i].arg_count, HEAPSTAT_MISC); On 2015/08/04 20:14:13, zhaoqin wrote: > nit, we often use things like sizeof(iter->targets[i].arg_values[0]) to get the > size, and it works even after we change the type of the arg_values. Done. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.c#newcode626 drfuzz/drfuzz.c:626: capture_crash(dcontext, &fp->current_exception_chain->first, crash); On 2015/08/04 20:14:13, zhaoqin wrote: > so on the first exception, the ->last is left there? > should it be last be also initialized it too. > Hmm, then there might be complication of two pointer pointing to the same > struct, should avoid double free. > > Is it possible just remember the last? I think it would be simpler. Added memcpy into the last (they are not pointers). https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode189 drfuzz/drfuzz.h:189: * Register a crash notification callback, which is called by drfuzz when an exception On 2015/08/04 20:14:13, zhaoqin wrote: > I feel that maybe early thread terminate callback is a better name, as we call > this callback if the fuzz chain is not NULL, i.e., early terminate, not even > necessary caused by the exception. The exception is cleared on the next fuzz iteration, so the callback should only occur if the fuzz target raised an exception that resulted in thread termination. We skip the callback if (a) the exception was caught and prevented termination of the thread, or (b) if code outside the fuzz target raised the exception. If an exception is caught and handled during the very last fuzz iteration, we may get confused and report it as a thread crash. There is an XXX note about that, with suggestions for how to fix it. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode192 drfuzz/drfuzz.h:192: * drfuzz_target_iterator_next(), but do not free the iterator (it has been pre-started). On 2015/08/04 20:14:13, zhaoqin wrote: > you mean the caller will call the _stop instead? yes--fixed the comment https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode193 drfuzz/drfuzz.h:193: */ On 2015/08/04 20:14:13, zhaoqin wrote: > we also want to say if we want to support multiple callbacks, and what's the > order if there are more than one. Done. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode197 drfuzz/drfuzz.h:197: drfuzz_exception_t *last_exception, On 2015/08/04 20:14:13, zhaoqin wrote: > do you think if we want these two args. > From the implementation, it looks like we will have an uninit last_exception > value, at least it should be NULL if one exception happens, then which one > should be NULL, first or last? > > Maybe just keep last? When we talked with Derek we agreed to have both first and last. Should we discuss again? It's all the same to me. I changed the implementation to set both first and last to the first exception, so neither will be uninit. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode240 drfuzz/drfuzz.h:240: * frames, and use drfuzz_target_iterator_free() to free the iterator and all frames. On 2015/08/04 20:14:13, zhaoqin wrote: > _stop. Done. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode249 drfuzz/drfuzz.h:249: drfuzz_target_frame_t * On 2015/08/04 20:14:13, zhaoqin wrote: > typically we return DRMF_STATUS_T, kind of strange we change only this one, > but it does not make much sense to return DRMF_STATUS_T here either. Acknowledged. https://codereview.appspot.com/257120043/diff/60001/make/git/git_review.sh File make/git/git_review.sh (right): https://codereview.appspot.com/257120043/diff/60001/make/git/git_review.sh#ne... make/git/git_review.sh:123: -t "${subject}" -m "${msg}" ${email} HEAD^1) On 2015/08/04 20:14:13, zhaoqin wrote: > I think I changed that already. Yes, so the change will disappear when I rebase before committing. https://codereview.appspot.com/257120043/diff/60001/tests/framework/drfuzz_cl... File tests/framework/drfuzz_client_empty.c (right): https://codereview.appspot.com/257120043/diff/60001/tests/framework/drfuzz_cl... tests/framework/drfuzz_client_empty.c:52: On 2015/08/04 20:14:13, zhaoqin wrote: > ? Artifact of adding and removing lines. No diff on this file now.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/257120043/diff/60001/common/utils.h File common/utils.h (right): https://codereview.appspot.com/257120043/diff/60001/common/utils.h#newcode814 common/utils.h:814: * CALLBACK LIST On 2015/08/04 22:50:11, Byron wrote: > On 2015/08/04 20:14:13, zhaoqin wrote: > > On 2015/08/04 17:18:55, Byron wrote: > > > This whole thing could be a separate CL, though it's fairly trivial. > > > > I was thinking about only support one callback, so a single pointer would do > the > > work, and maybe we can add support for multiple ones later, but I guess we can > > add them now. > > > > Then do we need worry about the ordering, or the order is the reverse order of > > the calling? > > I'd say let's add priorities later, if we need them. Since we're going to do a release, unless we go to efforts to hide drfuzz, we won't be able to change interfaces: so make sure you bake in enough flexibility for that now.
Sign in to reply to this message.
As the API may affect the release, I want to have a more discussion about it. Please see my comment about the APIs. Derek, you might also want to take a look. https://codereview.appspot.com/257120043/diff/60001/common/utils.h File common/utils.h (right): https://codereview.appspot.com/257120043/diff/60001/common/utils.h#newcode814 common/utils.h:814: * CALLBACK LIST On 2015/08/05 00:10:02, bruening wrote: > On 2015/08/04 22:50:11, Byron wrote: > > On 2015/08/04 20:14:13, zhaoqin wrote: > > > On 2015/08/04 17:18:55, Byron wrote: > > > > This whole thing could be a separate CL, though it's fairly trivial. > > > > > > I was thinking about only support one callback, so a single pointer would do > > the > > > work, and maybe we can add support for multiple ones later, but I guess we > can > > > add them now. > > > > > > Then do we need worry about the ordering, or the order is the reverse order > of > > > the calling? > > > > I'd say let's add priorities later, if we need them. > > Since we're going to do a release, unless we go to efforts to hide drfuzz, we > won't be able to change interfaces: so make sure you bake in enough flexibility > for that now. The code here are internal ones, we do not have to worry about it. The one to worry about are the exposed ones. https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/60001/drfuzz/drfuzz.h#newcode189 drfuzz/drfuzz.h:189: * Register a crash notification callback, which is called by drfuzz when an exception On 2015/08/04 22:50:11, Byron wrote: > On 2015/08/04 20:14:13, zhaoqin wrote: > > I feel that maybe early thread terminate callback is a better name, as we call > > this callback if the fuzz chain is not NULL, i.e., early terminate, not even > > necessary caused by the exception. > > The exception is cleared on the next fuzz iteration, so the callback should only > occur if the fuzz target raised an exception that resulted in thread > termination. We skip the callback if (a) the exception was caught and prevented > termination of the thread, or (b) if code outside the fuzz target raised the > exception. > > If an exception is caught and handled during the very last fuzz iteration, we > may get confused and report it as a thread crash. There is an XXX note about > that, with suggestions for how to fix it. Yes, I agree the situation above. I am talking about the third situation, i.e., the fuzz target function exit instead of return without an exception. For example, the code inside of the target function is something like: function_being_fuzzed(input_value) { if (input_value_is_wrong) exit() ... return. } There would be no exception, and the thread exit, meanwhile, the live target call chain is not NULL, so we will call the crash_cb even no exception happens. That's why I suggest we either not calling the callback, or we change the callback name to early_termination or something. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode70 drfuzz/drfuzz.h:70: } drfuzz_target_frame_t; ok, user exposed data structure, do we want to add anything in the future. One thing I can think of is the mutated value we put for current iteration. I guess we can add some new api to let the client to query the fuzz value based on the func_pc. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode86 drfuzz/drfuzz.h:86: thread_id_t thread_id; ok, since this gonna be exposed, we want to be more careful. One big thing I think it might be useful is the user_data field. DrM may want to add a callstack to the exception as user_data, but that would requires a callback so we can call it to delete the user_data when drfuzz delete the exception records. It seems a pain to add such support, so I am ok without it. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode193 drfuzz/drfuzz.h:193: * Multiple registration is allowed; they will be called in reverse order of registration. now here we need discuss about the API. If we support multiple callback, it seems that we should support the ordering. Meanwhile, we need handle the target iterator right, i.e., after each callback, we want to reset it back so that the next callback will correctly iterate the targets. Or since we only support one pre/post fuzz, it may make sense to support only one callback, and let the client writer to worry about how to order multiple components callbacks. Multiple call of the registration function will overwrites the previous one, and registration with NULL is the un-registration. As for the iterator, would it be easier to let the caller to start and stop, maybe we just pass in the per-thread drfuzz_cxt? As for the exception, as I mentioned above, there might be a chance that no exception happens at all, and most likely there would be one exception. Maybe we can have an array for the exception info, i.e., drfuzz_register_thread_early_termination(void (*callback) (drfuzz_cxt, uint excption_count, drfuzz_exception_t *exceptions)) The callback and iterate the exceptions based on the count, and we can add more exceptions if we want. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode244 drfuzz/drfuzz.h:244: drfuzz_target_iterator_start(); do we want to pass in the drfuzz_cxt. On thread exit, the current drcontext might not be your thread's drcontext. It is safe to use a passed in drfuzz_cxt instead. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode258 drfuzz/drfuzz.h:258: drfuzz_target_iterator_stop(drfuzz_target_iterator_t *iter); ditto about the drfuzz_cxt.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode70 drfuzz/drfuzz.h:70: } drfuzz_target_frame_t; On 2015/08/05 16:21:49, zhaoqin wrote: > ok, user exposed data structure, do we want to add anything in the future. > > One thing I can think of is the mutated value we put for current iteration. > I guess we can add some new api to let the client to query the fuzz value based > on the func_pc. Assuming the buffer is one of the arguments to a target, we have this API already. They can just call drfuzz_get_arg() with the target_pc and original = false (i.e., get the fuzzed value). https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode86 drfuzz/drfuzz.h:86: thread_id_t thread_id; On 2015/08/05 16:21:49, zhaoqin wrote: > ok, since this gonna be exposed, we want to be more careful. > > One big thing I think it might be useful is the user_data field. > DrM may want to add a callstack to the exception as user_data, but that would > requires a callback so we can call it to delete the user_data when drfuzz delete > the exception records. > It seems a pain to add such support, so I am ok without it. Seems like we should have it. Added the callbacks. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode193 drfuzz/drfuzz.h:193: * Multiple registration is allowed; they will be called in reverse order of registration. On 2015/08/05 16:21:49, zhaoqin wrote: > now here we need discuss about the API. > If we support multiple callback, it seems that we should support the ordering. > Meanwhile, we need handle the target iterator right, i.e., after each callback, > we want to reset it back so that the next callback will correctly iterate the > targets. This is a bug. Fixed it. > > Or since we only support one pre/post fuzz, it may make sense to support only > one callback, and let the client writer to worry about how to order multiple > components callbacks. > Multiple call of the registration function will overwrites the previous one, and > registration with NULL is the un-registration. That seems like a lot of work for the client. I'd prefer to allow multiples. > > As for the iterator, would it be easier to let the caller to start and stop, > maybe we just pass in the per-thread drfuzz_cxt? This is not a possibility because the exception-live targets must be frozen into a snapshot. Instead of making another data structure, it is much easier to just put the targets into an iterator. But now drfuzz must be responsible for stopping, since it did the starting. > > As for the exception, as I mentioned above, there might be a chance that no > exception happens at all, and most likely there would be one exception. > Maybe we can have an array for the exception info, i.e., > > drfuzz_register_thread_early_termination(void (*callback) > (drfuzz_cxt, > uint excption_count, > drfuzz_exception_t *exceptions)) > The callback and iterate the exceptions based on the count, and we can add more > exceptions if we want. > This would be fine, though what if the chain gets enormous, e.g. a recursive function iterates 1000 times and then exits by throw/catch in each stack frame? We don't want to crash with "out of memory" while handling a target crash. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode244 drfuzz/drfuzz.h:244: drfuzz_target_iterator_start(); On 2015/08/05 16:21:49, zhaoqin wrote: > do we want to pass in the drfuzz_cxt. > > On thread exit, the current drcontext might not be your thread's drcontext. > It is safe to use a passed in drfuzz_cxt instead. > This function always returns an empty iterator on thread exit, because nothing is live on the call stack. Instead, the client must use the iterator in the crash callback, which is a snapshot of the targets that was taken at the time of the first exception. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode258 drfuzz/drfuzz.h:258: drfuzz_target_iterator_stop(drfuzz_target_iterator_t *iter); On 2015/08/05 16:21:49, zhaoqin wrote: > ditto about the drfuzz_cxt. The client needs to query targets from the thread the targets are running on. Otherwise we have to add locks and deal with various other complications.
Sign in to reply to this message.
continue discussion on the API. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode70 drfuzz/drfuzz.h:70: } drfuzz_target_frame_t; On 2015/08/05 17:50:05, Byron wrote: > On 2015/08/05 16:21:49, zhaoqin wrote: > > ok, user exposed data structure, do we want to add anything in the future. > > > > One thing I can think of is the mutated value we put for current iteration. > > I guess we can add some new api to let the client to query the fuzz value > based > > on the func_pc. > > Assuming the buffer is one of the arguments to a target, we have this API > already. They can just call drfuzz_get_arg() with the target_pc and original = > false (i.e., get the fuzzed value). hmm, then why do we need arg_values here, I guess it is simpler to pass it here if it is often used. https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode244 drfuzz/drfuzz.h:244: drfuzz_target_iterator_start(); On 2015/08/05 17:50:05, Byron wrote: > On 2015/08/05 16:21:49, zhaoqin wrote: > > do we want to pass in the drfuzz_cxt. > > > > On thread exit, the current drcontext might not be your thread's drcontext. > > It is safe to use a passed in drfuzz_cxt instead. > > > > This function always returns an empty iterator on thread exit, because nothing > is live on the call stack. Instead, the client must use the iterator in the > crash callback, which is a snapshot of the targets that was taken at the time of > the first exception. then how could a client know the case we discussed before: the fuzz target function or its callee calls exit instead of abort or signal to exit. [12:44] <byronh> The user can register a thread exit callback for that. [12:45] <byronh> During the standard thread exit callback, they can check for live fuzz targets. which API should the the client use to know if any live fuzz target left on the callstack? I thought this iterator is used for that https://codereview.appspot.com/257120043/diff/80001/drfuzz/drfuzz.h#newcode258 drfuzz/drfuzz.h:258: drfuzz_target_iterator_stop(drfuzz_target_iterator_t *iter); On 2015/08/05 17:50:05, Byron wrote: > On 2015/08/05 16:21:49, zhaoqin wrote: > > ditto about the drfuzz_cxt. > > The client needs to query targets from the thread the targets are running on. > Otherwise we have to add locks and deal with various other complications. the drfuzz_cxt or drcontext is the thread private data structure. It could only be used by other thread only process exit, where the whole execution is suspended, and one thread calling thread exit on behave of other thread. If the client keep the drcontext, and use it in another thread, DR won't handle that either. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.c#newcode302 drfuzz/drfuzz.c:302: fuzz_pass_context_t *fp = (fuzz_pass_context_t *) drmgr_get_tls_field(dcontext, we may want to generalize this fp to be drfuzz_cxt and pass it everywhere. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode197 drfuzz/drfuzz.h:197: drfuzz_register_exception_callback(void (*exc_cb)(drfuzz_exception_t *exc)); drm need mcontext to construct the callstack, we must provide such info because the app mcontext might be different from the actual mcontext. So basically, you need pass dr_exception_t/dr_siginfo_t to callback. Also, you want to pass drfuzz_cxt to the callback, so it can get drcontext or anything from the drfuzz_cxt. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode213 drfuzz/drfuzz.h:213: * Multiple registration is allowed; they will be called in reverse order of registration. ok, so we decide to support multiple callbacks but using the registration order to decide the calling order. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode218 drfuzz/drfuzz.h:218: drfuzz_exception_t *last_exception, what do you think about an array instead of two separate args? https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode234 drfuzz/drfuzz.h:234: * callback to free any \p user_data that was added to the exception. add comment about if we support multiple callbacks and the order. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode244 drfuzz/drfuzz.h:244: drfuzz_unregister_exception_delete_callback(void (*exc_del_cb)(drfuzz_exception_t *exc)); nit, move these two to after unregister_exception_callback, seems more related.
Sign in to reply to this message.
https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.c#newcode302 drfuzz/drfuzz.c:302: fuzz_pass_context_t *fp = (fuzz_pass_context_t *) drmgr_get_tls_field(dcontext, On 2015/08/05 20:29:15, zhaoqin wrote: > we may want to generalize this fp to be drfuzz_cxt and pass it everywhere. Done. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode197 drfuzz/drfuzz.h:197: drfuzz_register_exception_callback(void (*exc_cb)(drfuzz_exception_t *exc)); On 2015/08/05 20:29:15, zhaoqin wrote: > drm need mcontext to construct the callstack, we must provide such info because > the app mcontext might be different from the actual mcontext. > So basically, you need pass dr_exception_t/dr_siginfo_t to callback. > Also, you want to pass drfuzz_cxt to the callback, so it can get drcontext or > anything from the drfuzz_cxt. fuzz_pass_context_t is now the client's opaque fuzzcxt, and the wrapcxt and dcontext are inside it. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode213 drfuzz/drfuzz.h:213: * Multiple registration is allowed; they will be called in reverse order of registration. On 2015/08/05 20:29:15, zhaoqin wrote: > ok, so we decide to support multiple callbacks but using the registration order > to decide the calling order. Changed to explicit priorities by adding priority support to the generic callback list. Also added a test for the callback list. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode218 drfuzz/drfuzz.h:218: drfuzz_exception_t *last_exception, On 2015/08/05 20:29:15, zhaoqin wrote: > what do you think about an array instead of two separate args? Changed it to an array, but only of size 2 for first and last. Can change that later without breaking the API. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode234 drfuzz/drfuzz.h:234: * callback to free any \p user_data that was added to the exception. On 2015/08/05 20:29:15, zhaoqin wrote: > add comment about if we support multiple callbacks and the order. Done. https://codereview.appspot.com/257120043/diff/100001/drfuzz/drfuzz.h#newcode244 drfuzz/drfuzz.h:244: drfuzz_unregister_exception_delete_callback(void (*exc_del_cb)(drfuzz_exception_t *exc)); On 2015/08/05 20:29:15, zhaoqin wrote: > nit, move these two to after unregister_exception_callback, seems more related. Done.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. It also adds a unit test for the new generic priority list. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
Uploaded a new version with better handling of callback priorities, and a better unit test with proper abort on error.
Sign in to reply to this message.
LGTM with some comment The biggest one is the priority struct and utils.h, but should not need another look from me. https://codereview.appspot.com/257120043/diff/140001/common/utils.h File common/utils.h (right): https://codereview.appspot.com/257120043/diff/140001/common/utils.h#newcode962 common/utils.h:962: callback_list_create(heapstat_t type, void *lock); why pass in lock, would a bool and the list create its own lock better? https://codereview.appspot.com/257120043/diff/140001/common/utils.h#newcode966 common/utils.h:966: callback_list_delete(callback_list_t *list); destroy? https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c File common/utils_shared.c (right): https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:127: callback_list_node_t *node) some comment about this function would be helpful https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:145: strcmp(node_array[i]->priority->name, node->priority->before) == 0) { should we check if name is NULL? https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:167: callback_list_sort(callback_list_t *list) some comment about this function would be helpful to understand how the list are sorted. https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:226: callback_list_delete(callback_list_t *list) delete and _remove later is kind of confusing, maybe destroy is better? https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:238: callback_priority_t *priority) could we support NULL as default priority here? https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:243: ASSERT(priority->struct_size <= sizeof(callback_priority_t), "incompatible priority"); you use priority here but check if it is NULL later. https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.c#newcode102 drfuzz/drfuzz.c:102: #define EXCEPTION_IS_EMPTY(e) ((e)->fault_pc == 0) we could have case that a fault_pc is 0 when jump to NULL. https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.c#newcode718 drfuzz/drfuzz.c:718: if (!EXCEPTION_IS_EMPTY(&LAST_EXCEPTION(fp))) we could just use exception_count to detect, right? https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode41 drfuzz/drfuzz.h:41: #include "utils.h" /* for callback lists */ hmm, I am not sure if utils.h is really an exposed header, I think it is more for internal use. https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode241 drfuzz/drfuzz.h:241: void (*pre_fuzz_cb)(generic_func_t target_pc, void *fuzzcxt, hmm, could we put fuzzcxt be the first? https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode243 drfuzz/drfuzz.h:243: bool (*post_fuzz_cb)(generic_func_t target_pc, void *fuzzcxt, ditto https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode262 drfuzz/drfuzz.h:262: callback_priority_t *priority); alignment if we do not include util.h, we need put callback_priority_t somewhere, e.g., framework's public.h could I pass NULL for priority, it would be nice to be yes. If yes, what's the default value for the rank? https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode286 drfuzz/drfuzz.h:286: callback_priority_t *priority); ditto https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode310 drfuzz/drfuzz.h:310: callback_priority_t *priority); ditto https://codereview.appspot.com/257120043/diff/140001/make/git/git_review.sh File make/git/git_review.sh (right): https://codereview.appspot.com/257120043/diff/140001/make/git/git_review.sh#n... make/git/git_review.sh:123: -t "${subject}" -m "${msg}" ${email} HEAD^1) it has been fixed.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. It also adds a unit test for the new generic priority list. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
bruening's turn to review https://codereview.appspot.com/257120043/diff/140001/common/utils.h File common/utils.h (right): https://codereview.appspot.com/257120043/diff/140001/common/utils.h#newcode962 common/utils.h:962: callback_list_create(heapstat_t type, void *lock); On 2015/08/10 03:17:56, zhaoqin wrote: > why pass in lock, would a bool and the list create its own lock better? Done. https://codereview.appspot.com/257120043/diff/140001/common/utils.h#newcode966 common/utils.h:966: callback_list_delete(callback_list_t *list); On 2015/08/10 03:17:56, zhaoqin wrote: > destroy? Done. https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c File common/utils_shared.c (right): https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:127: callback_list_node_t *node) On 2015/08/10 03:17:56, zhaoqin wrote: > some comment about this function would be helpful Done. https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:145: strcmp(node_array[i]->priority->name, node->priority->before) == 0) { On 2015/08/10 03:17:56, zhaoqin wrote: > should we check if name is NULL? Done. https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:167: callback_list_sort(callback_list_t *list) On 2015/08/10 03:17:56, zhaoqin wrote: > some comment about this function would be helpful to understand how the list are > sorted. Done. https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:226: callback_list_delete(callback_list_t *list) On 2015/08/10 03:17:56, zhaoqin wrote: > delete and _remove later is kind of confusing, maybe destroy is better? Done. https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:238: callback_priority_t *priority) On 2015/08/10 03:17:56, zhaoqin wrote: > could we support NULL as default priority here? Done. https://codereview.appspot.com/257120043/diff/140001/common/utils_shared.c#ne... common/utils_shared.c:243: ASSERT(priority->struct_size <= sizeof(callback_priority_t), "incompatible priority"); On 2015/08/10 03:17:56, zhaoqin wrote: > you use priority here but check if it is NULL later. Done. https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.c#newcode102 drfuzz/drfuzz.c:102: #define EXCEPTION_IS_EMPTY(e) ((e)->fault_pc == 0) On 2015/08/10 03:17:56, zhaoqin wrote: > we could have case that a fault_pc is 0 when jump to NULL. Removed in favor of checking by exception count. https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.c#newcode718 drfuzz/drfuzz.c:718: if (!EXCEPTION_IS_EMPTY(&LAST_EXCEPTION(fp))) On 2015/08/10 03:17:56, zhaoqin wrote: > we could just use exception_count to detect, right? Done. https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode41 drfuzz/drfuzz.h:41: #include "utils.h" /* for callback lists */ On 2015/08/10 03:17:56, zhaoqin wrote: > hmm, I am not sure if utils.h is really an exposed header, I think it is more > for internal use. Done. https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode262 drfuzz/drfuzz.h:262: callback_priority_t *priority); On 2015/08/10 03:17:56, zhaoqin wrote: > alignment > > if we do not include util.h, we need put callback_priority_t somewhere, e.g., > framework's public.h > > could I pass NULL for priority, it would be nice to be yes. > If yes, what's the default value for the rank? Done. https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode286 drfuzz/drfuzz.h:286: callback_priority_t *priority); On 2015/08/10 03:17:56, zhaoqin wrote: > ditto Done. https://codereview.appspot.com/257120043/diff/140001/drfuzz/drfuzz.h#newcode310 drfuzz/drfuzz.h:310: callback_priority_t *priority); On 2015/08/10 03:17:56, zhaoqin wrote: > ditto Done.
Sign in to reply to this message.
https://codereview.appspot.com/257120043/diff/160001/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/160001/drfuzz/drfuzz.c#newcode793 drfuzz/drfuzz.c:793: notify_exception(fuzz_pass_context_t *fp, drfuzz_crash_info_t *info, These callback iterators would benefit from a template since I think they'll always be the same. Would that be worth the complexity of having a template? Or is there a lighter way to genericize the operation? The tricky part is the variable number and type of args in the callback invocation. https://codereview.appspot.com/257120043/diff/160001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/160001/drfuzz/drfuzz.h#newcode279 drfuzz/drfuzz.h:279: * \note For multiple registration, honors named before/after priorities where This long comment paragraph is repeated several times. Is there a way to put it in a shared location?
Sign in to reply to this message.
https://codereview.appspot.com/257120043/diff/160001/framework/public.h File framework/public.h (right): https://codereview.appspot.com/257120043/diff/160001/framework/public.h#newco... framework/public.h:109: } callback_priority_t; drfuzz uses drmgr already. Can we use drmgr_priority_t here rather than having an almost-identical struct?
Sign in to reply to this message.
https://codereview.appspot.com/257120043/diff/160001/common/utils_shared.c File common/utils_shared.c (right): https://codereview.appspot.com/257120043/diff/160001/common/utils_shared.c#ne... common/utils_shared.c:170: * by the named priorities, such that the edges are name->before and after->name. Nodes "name->before"?? https://codereview.appspot.com/257120043/diff/160001/common/utils_shared.c#ne... common/utils_shared.c:174: * with NULL priority are treated like nodes having no named priorities and rank 0. Hmm, this seems like overkill. I would say, just do what drmgr does and fail if unsatisfiable priorities are given. IMHO it doesn't buy us anything to have complex code to support cycles and in fact is a negative due to complex code -- do you have a real scenario in mind where someone needs to specify a cycle? Wouldn't it be better to fail if someone accidentally requests a cycle, and that would involve simpler code here as well, right? https://codereview.appspot.com/257120043/diff/160001/common/utils_shared.c#ne... common/utils_shared.c:177: callback_list_sort(callback_list_t *list) Seems like insertion sort as each callback is added should be just fine instead. Should we just share the drmgr code in some manner?
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. It also adds a single-threaded test of the new fault handling API. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
I made some major changes, so I'm not sure if Derek or Qin wants to review now. The callback list has been completely removed. Callbacks in drfuzz only allow a single registration. There is a new test for the basic functionality of the fault handling and user data. The term "exception" has been changed to the term "fault" in drfuzz.[ch] because (a) it is shorter, and (b) it seems like a more general term, since "exception" is mainly a Windows term. https://codereview.appspot.com/257120043/diff/160001/common/utils_shared.c File common/utils_shared.c (right): https://codereview.appspot.com/257120043/diff/160001/common/utils_shared.c#ne... common/utils_shared.c:170: * by the named priorities, such that the edges are name->before and after->name. Nodes On 2015/08/11 01:46:31, bruening wrote: > "name->before"?? Removed this feature. https://codereview.appspot.com/257120043/diff/160001/common/utils_shared.c#ne... common/utils_shared.c:174: * with NULL priority are treated like nodes having no named priorities and rank 0. On 2015/08/11 01:46:31, bruening wrote: > Hmm, this seems like overkill. I would say, just do what drmgr does and fail if > unsatisfiable priorities are given. IMHO it doesn't buy us anything to have > complex code to support cycles and in fact is a negative due to complex code -- > do you have a real scenario in mind where someone needs to specify a cycle? > Wouldn't it be better to fail if someone accidentally requests a cycle, and that > would involve simpler code here as well, right? Removed this feature. https://codereview.appspot.com/257120043/diff/160001/common/utils_shared.c#ne... common/utils_shared.c:177: callback_list_sort(callback_list_t *list) On 2015/08/11 01:46:31, bruening wrote: > Seems like insertion sort as each callback is added should be just fine instead. > > Should we just share the drmgr code in some manner? Removed this feature. https://codereview.appspot.com/257120043/diff/160001/framework/public.h File framework/public.h (right): https://codereview.appspot.com/257120043/diff/160001/framework/public.h#newco... framework/public.h:109: } callback_priority_t; On 2015/08/10 15:45:45, bruening wrote: > drfuzz uses drmgr already. Can we use drmgr_priority_t here rather than having > an almost-identical struct? Removed this feature.
Sign in to reply to this message.
LGTM w/ comments https://codereview.appspot.com/257120043/diff/180001/common/utils.h File common/utils.h (right): https://codereview.appspot.com/257120043/diff/180001/common/utils.h#newcode942 common/utils.h:942: /* note: Guarantees that even if src overflows n, the allocated buffer will be s/n/max/ https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode103 drfuzz/drfuzz.c:103: #define MAX_CHAINED_FAULTS 2 Please add comments. This is the max size of the faults array? Why does it say "CHAINED"? https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode105 drfuzz/drfuzz.c:105: #define LAST_FAULT(fp) ((fp)->thread_state->faults[1]) This seems to assume a max of 2 -- yet it makes no use of MAX_CHAINED_FAULTS nor does it have a comment saying to update it when the max is updated. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode106 drfuzz/drfuzz.c:106: #define SIZEOF_FAULTS (2 * sizeof(drfuzz_fault_t)) What is this hardcoded 2? Shouldn't this be MAX_CHAINED_FAULTS? Comments and maybe better names would help: how about ARRAY in the name or sthg, and how about having similar names for the max and this sizeof? https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode308 drfuzz/drfuzz.c:308: /* crash is indicated either by unhandled fault, or by terminated fuzz targets */ What you want is DR i#966, right? Perhaps a xref here. Also, you want the app exit code to help diagnose a crash: xref i#1260. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode841 drfuzz/drfuzz.c:841: * critical, and extend the default set to include e.g. SIGBUS, SIGABRT, etc. We have to include SIGBUS as on Linux it is a not-insignificant source of crashes that fuzzers will hit: if you just de-ref random memory you'll hit it enough that you want to include it. Less likely when fuzzing but still direct crashes from the app are SIGILL and STATUS_ILLEGAL_INSTRUCTION (along w/ some other rarer exception codes on Windows). https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode80 drfuzz/drfuzz.h:80: /** Check the generated docs: on Windows does doxygen put this on the typedef or not? https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode81 drfuzz/drfuzz.h:81: * Provides context information about a critical fault. On Unix cast to dr_siginfo_t, # for link https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode82 drfuzz/drfuzz.h:82: * or on Windows cast to dr_exception_t. See documentation on those structs for details. # for link https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode91 drfuzz/drfuzz.h:91: * Provides details about a single occurrence of a critical fault (i.e., a UNIX signal or What is a "critical fault"? It says "a UNIX signal or a Windows exception" -- but many signals are unrelated to any kind of "fault" (e.g., SIGUSR1, real-time signals, and would you call a timer alarm a "fault"?) This could be described more precisely. In isolation, if I header "critical fault", I would think it meant a hardware-generated fault (x86 uses the term "fault" to mean something precise: it is a hardware exception whose return point is the start of the faulting instruction, as opposed to a trap or abort) that was not caught and led to process termination. But you don't know whether it will be caught up front, right? https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode92 drfuzz/drfuzz.h:92: * or a Windows exception). See comments on drfuzz_register_fault_event(). In the case of or or https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode95 drfuzz/drfuzz.h:95: * the app continue to run. continued https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode97 drfuzz/drfuzz.h:97: typedef struct _drfuzz_fault_t { These names are very similar: "drfuzz_fault_info_t" and "drfuzz_fault_t". It's not clear what their relationship is. Maybe one or both could be renamed -- drfuzz_fault_arch_info_t and drfuzz_fault_base_info_t? https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode99 drfuzz/drfuzz.h:99: * Signal number (Unix) or exception code (Windows). This duplicates info in drfuzz_fault_info_t? I'm confused about this struct vs that one. Please try to clarify in the comments. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode260 drfuzz/drfuzz.h:260: * fuzz target encounters a critical fault. The \p fault gives basic information about So is the idea that drfuzz records info on each fault when it happens and doesn't know if the app will catch it, and then at process termination due to an unhandled fault it tells the user about all the faults it saw? Maybe this could explain it like that. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode261 drfuzz/drfuzz.h:261: * the kind and location of the fault, and may be retained by the callee. More detailed Until when? Until drfuzz_register_fault_delete_callback() is called? https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode288 drfuzz/drfuzz.h:288: * free any custom data attached to the fault's user_data field. Doesn't this also tell the user when it is no longer safe to refer to a drfuzz_fault_t? https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode308 drfuzz/drfuzz.h:308: * time of the fault. If the crash was not caused by this thread, the state may contain So you're going to go suspend all the other threads, it sounds like, and iterate over them, calling this for each https://codereview.appspot.com/257120043/diff/180001/tests/framework/drfuzz_a... File tests/framework/drfuzz_app_segfault.c (right): https://codereview.appspot.com/257120043/diff/180001/tests/framework/drfuzz_a... tests/framework/drfuzz_app_segfault.c:45: /* A crash prone function. Segfaults with index out of range. */ This does not seem overly likely to actually segfault: the index has to be quite large to reach an unreadable page. Dr. Memory will report non-crashing errors for us, though.
Sign in to reply to this message.
Committed as https://github.com/DynamoRIO/drmemory/commit/32827fa8d17e476faa4b11b7d6bafe0e... Final commit log: --------------- i#1734 Create Dr. Fuzz: add exception API This CL adds the exception API, along with a general iterator for walking the set of fuzz targets currently live on the call stack. It also adds a single-threaded test of the new fault handling API, and the Dr. Fuzz entries in the doxygen tree. Review-URL: https://codereview.appspot.com/257120043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/257120043/diff/180001/common/utils.h File common/utils.h (right): https://codereview.appspot.com/257120043/diff/180001/common/utils.h#newcode942 common/utils.h:942: /* note: Guarantees that even if src overflows n, the allocated buffer will be On 2015/08/12 03:59:24, bruening wrote: > s/n/max/ Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c File drfuzz/drfuzz.c (right): https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode103 drfuzz/drfuzz.c:103: #define MAX_CHAINED_FAULTS 2 On 2015/08/12 03:59:24, bruening wrote: > Please add comments. This is the max size of the faults array? Why does it say > "CHAINED"? Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode105 drfuzz/drfuzz.c:105: #define LAST_FAULT(fp) ((fp)->thread_state->faults[1]) On 2015/08/12 03:59:24, bruening wrote: > This seems to assume a max of 2 -- yet it makes no use of MAX_CHAINED_FAULTS nor > does it have a comment saying to update it when the max is updated. Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode106 drfuzz/drfuzz.c:106: #define SIZEOF_FAULTS (2 * sizeof(drfuzz_fault_t)) On 2015/08/12 03:59:24, bruening wrote: > What is this hardcoded 2? Shouldn't this be MAX_CHAINED_FAULTS? > > Comments and maybe better names would help: how about ARRAY in the name or sthg, > and how about having similar names for the max and this sizeof? Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode308 drfuzz/drfuzz.c:308: /* crash is indicated either by unhandled fault, or by terminated fuzz targets */ On 2015/08/12 03:59:24, bruening wrote: > What you want is DR i#966, right? Perhaps a xref here. Also, you want the app > exit code to help diagnose a crash: xref i#1260. Changed to report crash anytime a thread exits with live fuzz targets, regardless of faults. This assumes an actual crash will abort fuzz targets on all other threads, so we don't have to make that happen. Could add a separate test for it. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.c#newcode841 drfuzz/drfuzz.c:841: * critical, and extend the default set to include e.g. SIGBUS, SIGABRT, etc. On 2015/08/12 03:59:24, bruening wrote: > We have to include SIGBUS as on Linux it is a not-insignificant source of > crashes that fuzzers will hit: if you just de-ref random memory you'll hit it > enough that you want to include it. Less likely when fuzzing but still direct > crashes from the app are SIGILL and STATUS_ILLEGAL_INSTRUCTION (along w/ some > other rarer exception codes on Windows). Added SIGBUS for now. Can extend later. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h File drfuzz/drfuzz.h (right): https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode80 drfuzz/drfuzz.h:80: /** On 2015/08/12 03:59:24, bruening wrote: > Check the generated docs: on Windows does doxygen put this on the typedef or > not? It works. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode81 drfuzz/drfuzz.h:81: * Provides context information about a critical fault. On Unix cast to dr_siginfo_t, On 2015/08/12 03:59:24, bruening wrote: > # for link The link is not possible because dr_siginfo_t is defined in the submodule, which has a separate doxygen tree. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode82 drfuzz/drfuzz.h:82: * or on Windows cast to dr_exception_t. See documentation on those structs for details. On 2015/08/12 03:59:24, bruening wrote: > # for link Can't link to DR from Dr. Memory. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode91 drfuzz/drfuzz.h:91: * Provides details about a single occurrence of a critical fault (i.e., a UNIX signal or On 2015/08/12 03:59:24, bruening wrote: > What is a "critical fault"? It says "a UNIX signal or a Windows exception" -- > but many signals are unrelated to any kind of "fault" (e.g., SIGUSR1, real-time > signals, and would you call a timer alarm a "fault"?) This could be described > more precisely. In isolation, if I header "critical fault", I would think it > meant a hardware-generated fault (x86 uses the term "fault" to mean something > precise: it is a hardware exception whose return point is the start of the > faulting instruction, as opposed to a trap or abort) that was not caught and led > to process termination. But you don't know whether it will be caught up front, > right? Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode92 drfuzz/drfuzz.h:92: * or a Windows exception). See comments on drfuzz_register_fault_event(). In the case of On 2015/08/12 03:59:24, bruening wrote: > or or Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode95 drfuzz/drfuzz.h:95: * the app continue to run. On 2015/08/12 03:59:24, bruening wrote: > continued Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode97 drfuzz/drfuzz.h:97: typedef struct _drfuzz_fault_t { On 2015/08/12 03:59:24, bruening wrote: > These names are very similar: "drfuzz_fault_info_t" and "drfuzz_fault_t". It's > not clear what their relationship is. Maybe one or both could be renamed -- > drfuzz_fault_arch_info_t and drfuzz_fault_base_info_t? Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode97 drfuzz/drfuzz.h:97: typedef struct _drfuzz_fault_t { On 2015/08/12 03:59:24, bruening wrote: > These names are very similar: "drfuzz_fault_info_t" and "drfuzz_fault_t". It's > not clear what their relationship is. Maybe one or both could be renamed -- > drfuzz_fault_arch_info_t and drfuzz_fault_base_info_t? Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode99 drfuzz/drfuzz.h:99: * Signal number (Unix) or exception code (Windows). On 2015/08/12 03:59:24, bruening wrote: > This duplicates info in drfuzz_fault_info_t? I'm confused about this struct vs > that one. Please try to clarify in the comments. Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode260 drfuzz/drfuzz.h:260: * fuzz target encounters a critical fault. The \p fault gives basic information about On 2015/08/12 03:59:24, bruening wrote: > So is the idea that drfuzz records info on each fault when it happens and > doesn't know if the app will catch it, and then at process termination due to an > unhandled fault it tells the user about all the faults it saw? Maybe this could > explain it like that. Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode261 drfuzz/drfuzz.h:261: * the kind and location of the fault, and may be retained by the callee. More detailed On 2015/08/12 03:59:24, bruening wrote: > Until when? Until drfuzz_register_fault_delete_callback() is called? Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode288 drfuzz/drfuzz.h:288: * free any custom data attached to the fault's user_data field. On 2015/08/12 03:59:24, bruening wrote: > Doesn't this also tell the user when it is no longer safe to refer to a > drfuzz_fault_t? Done. https://codereview.appspot.com/257120043/diff/180001/drfuzz/drfuzz.h#newcode308 drfuzz/drfuzz.h:308: * time of the fault. If the crash was not caused by this thread, the state may contain On 2015/08/12 03:59:24, bruening wrote: > So you're going to go suspend all the other threads, it sounds like, and iterate > over them, calling this for each I'm assuming that a crash will abort all threads. So each thread will report its exceptions and/or live fuzz targets at thread exit. If the threads are not aborted, then as far as drfuzz is concerned, it wasn't a crash. https://codereview.appspot.com/257120043/diff/180001/tests/framework/drfuzz_a... File tests/framework/drfuzz_app_segfault.c (right): https://codereview.appspot.com/257120043/diff/180001/tests/framework/drfuzz_a... tests/framework/drfuzz_app_segfault.c:45: /* A crash prone function. Segfaults with index out of range. */ On 2015/08/12 03:59:24, bruening wrote: > This does not seem overly likely to actually segfault: the index has to be quite > large to reach an unreadable page. Dr. Memory will report non-crashing errors > for us, though. The test fuzzes the index arg with value 10000000, which I think will crash in any scenario for this tiny test app.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
