search

jirentabu / crashrpt

Automatically exported from code.google.com/p/crashrpt
0 stars 0 forks source link

Wrong handling of dwFlags when setting handlers #109

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
In CCrashHandler::SetProcess ExceptionHandlers, the handling of 
CR_INST_ALL_EXCEPTION_HANDLERS is wrong. It currently reads:

// Sets exception handlers that work on per-process basis
int CCrashHandler::SetProcessExceptionHandlers(DWORD dwFlags)
{
  crSetErrorMsg(_T("Unspecified error."));

  // If 0 is specified as dwFlags, assume all handlers should be
  // installed
  if((dwFlags&0x1FF)==0)
    dwFlags |= 0x1FFF;
  ...

where the if clause should obviously read (dwFlags&0x1FFF)==0. 
In fact, I would propose to define 

  #define CR_INST_ALL_EXCEPTION_HANDLERS 0x1FFF

because a |ed combination of CR_INST_ALL_EXCEPTION_HANDLERS with any other 
handler constant counter-intuitively now installs ONLY the other handler. 
Also it would allow something like:

  dwFlags = CR_INST_ALL_EXCEPTION_HANDLERS & ^CR_INST_NEW_OPERATOR_ERROR_HANDLER;

in order to install all but the new operator error handler. The check against 0 
could of course be maintained to not break any exsiting code.

On the other hand it could make sense to not install anything on process level 
if only thread-level handlers are required. 
This could then either be implemented by defining an additional flag like 
CR_INST_NO_EXCEPTION_HANDLERS which will prevent installation even if |ed with 
other constants or the zero could be used for that breaking backwards 
compatibility.

Original issue reported on code.google.com by Schoenle...@googlemail.com on 20 Sep 2011 at 8:34

GoogleCodeExporter commented 9 years ago 
The things you propose definitely should be implemented in some way. I'll think 
more about it and implement in the next release.

Original comment by zexspect...@gmail.com on 28 Sep 2011 at 2:44

GoogleCodeExporter commented 9 years ago 
Issue 108 has been merged into this issue.

Original comment by zexspect...@gmail.com on 9 Oct 2011 at 3:48

GoogleCodeExporter commented 9 years ago 
I decided to introduce new flag CR_INST_ALL_POSSIBLE_HANDLERS (0x1FFF) and make 
the old flag CR_INST_ALL_EXCEPTION_HANDLERS (0) deprecated: 

#define CR_INST_ALL_POSSIBLE_HANDLERS          0x1FFF  //!< Install all 
possible exception handlers.
#define CR_INST_ALL_EXCEPTION_HANDLERS         0       //!< Deprecated, not 
recommended to use. Use \ref CR_INST_ALL_POSSIBLE_HANDLERS instead.

I understand your idea with CR_INST_NO_EXCEPTION_HANDLERS flag, but this is not 
100% clean solution. Assume you need to install thread exception handlers only. 
But crInstall is designed to install thread-based exception handlers for the 
main thread, you can't install thread-based exception handlers for the main 
thread with crInstallToCurrentThread2(). So, you have to provide some 
handler-related flags for crInstall() function. Finally, I decided not to 
introduce the CR_INST_NO_EXCEPTION_HANDLERS flag becouse of the reason above.

Original comment by zexspect...@gmail.com on 9 Oct 2011 at 4:12

GoogleCodeExporter commented 9 years ago 
Fixed in v.1.3.0

Original comment by zexspect...@gmail.com on 22 Oct 2011 at 11:07