HPCC-13578 Prevent logging from destroyed crit secs from segfaulting #7336
Conversation
|
https://track.hpccsystems.com/browse/HPCC-13578 |
|
@richardkchapman please review. Note, setting the manger to NULL may have an effect - worth double checking. |
|
Automated Smoketest Build: success |
| @@ -2244,6 +2244,7 @@ MODULE_EXIT() | |||
| delete thePassNoneFilter; | |||
| delete thePassLocalFilter; | |||
| delete thePassAllFilter; | |||
| theManager = NULL; | |||
There was a problem hiding this comment.
What about the other ones?
|
I can't see how setting it to NULL (rather than leaving it pointing to released memory) will make anything worse... |
|
No nor I, but I thought I better point it out. |
| @@ -288,7 +288,15 @@ class CriticalSection | |||
| inline ~CriticalSection() | |||
| { | |||
| #ifdef _ASSERT_LOCK_SUPPORT | |||
| assertex(owner==0 && depth==0); | |||
There was a problem hiding this comment.
This feels like it might be adding some overhead to a very commonly used bit of code...
There was a problem hiding this comment.
On modern compilers try/catch should add no overhead to the code.
I could convert it to always use a fprintf instead, but potentially throwing an exception in a destructor isn't a good idea.
There was a problem hiding this comment.
Perhaps it should just be an assert?
There was a problem hiding this comment.
assert would only be triggered in debug mode (that may or may not be a good thing).
I have only ever hit this when interrupting a program. I would probably go for a printf, but I'll implement any. You can choose.
|
@ghalliday one minor comment |
|
@richardkchapman which option do you prefer? |
|
@ghalliday back to you |
|
I choose assert. That way we get the checking in debug builds without any overhead on release ones (since this is very commonly executed code) |
Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>
|
@richardkchapman changed to assert, and cleared the other variables. |
|
Automated Smoketest Build: success |
| #endif | ||
| } |
There was a problem hiding this comment.
Should there be an else fprintf(stderr...) ?
There was a problem hiding this comment.
I don't think so - nothing will have come out in the stack report either.
A good solution would be for DBGLOG to fallback to fprintf if the logMsgManager was NULL, but that would require a condition on every DBGLOG. That might in itself be a good change.
HPCC-13578 Prevent logging from destroyed crit secs from segfaulting Reviewed-By: Richard Chapman <rchapman@hpccsystems.com>
Signed-off-by: Gavin Halliday gavin.halliday@lexisnexis.com