[asterisk-bugs] [JIRA] (ASTERISK-24833) Logger concerns

[Matt Jordan (JIRA)]

    [ https://issues.asterisk.org/jira/browse/ASTERISK-24833?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=225178#comment-225178 ] 

Matt Jordan commented on ASTERISK-24833:
----------------------------------------

{quote}
The probability of these recursive loops is very low, but worth considering. A thread-local variable could allow ast_log/ast_verbose to ignore recursive calls and calls from the logger thread.
{quote}

I would say that any improvement that gets made should be subject to performance issues as well. Checking a threadlocal variable on every logging statement could get expensive.

{quote}
In theory trunk could stop using ast_str and ao2. Switching from ast_str would be a pain but straight forward and self contained. Removal of ao2 from the logging process would involve API/ABI changes. callid doesn't actually need to be an AO2 object. It could just be an int. This would require no ref-counting as the memory wouldn't be allocated. Thread storage would get an allocation to stop a copy of the int, but that allocation would take care of itself.
{quote}

I wouldn't mind seeing call-id become an atomic integer. It'd probably be a lot faster than the current implementation.

{quote}
Initialization order:
Everything uses the logger. Unfortunately the 'exec_after_rotate' option creates a dummy channel to do variable substitution. This pushes logging pretty far down the initialization list. Many start-up messages are lost. If the logger was initialized earlier it would require a way to block rotation until after it's safe to create a dummy channel for variable substitution.
{quote}

There are some *extremely* difficult subtle traps with changing the initialization of the logger. Consider that Asterisk may attempt to use the logger before it has forked itself, at which point any thread primitive that has been initialized is invalid in the forked process - and is liable to cause extremely difficult to debug problems. I would highly suggest not mucking around with logger initialization unless you have a very definitive plan in mind :-)

{quote}
I'm not sure if it's reasonable to avoid using channels (trunk only). This would involve having exec_after_rotation specifically replace ${filename}. We would also need to document that functions, expressions, and other variables are no longer supported. Would be nice if the new procedure produced an error if unsupported expressions are detected.
{quote}

I'm not sure there's a way to implement the {{exec_after_rotate}} functionality (with variable substitution, which appears to be a lot of the usefulness of it) without using a dummy channel. Consider that without variable substitution, an external script could do the work just as well. Removing it feels like a backwards break with existing systems that we probably shouldn't do unless we are sure that functionality isn't needed.

> Logger concerns
> ---------------
>
>                 Key: ASTERISK-24833
>                 URL: https://issues.asterisk.org/jira/browse/ASTERISK-24833
>             Project: Asterisk
>          Issue Type: Bug
>      Security Level: None
>          Components: Core/Logging
>    Affects Versions: SVN, 11.16.0, 13.2.0
>            Reporter: Corey Farrell
>            Severity: Minor
>
> I've been auditing startup order, and found some things about the logger that concern me.
> ast_log / ast_verbose do not protect against recursion:
> * __ast_str_helper can call ast_verbose during an allocation failure.
> * ao2 functions (for callid).  If an existing callid is unref'ed too many times, the object would be freed with threadstorage still pointing to it.  This could cause an infinite loop where ast_log tries to retrieve a callid, causing ao2 to report an error with ast_log.
> The probability of these recursive loops is very low, but worth considering.  A thread-local variable could allow ast_log/ast_verbose to ignore recursive calls and calls from the logger thread.
> In theory trunk could stop using ast_str and ao2.  Switching from ast_str would be a pain but straight forward and self contained.  Removal of ao2 from the logging process would involve API/ABI changes.  callid doesn't actually need to be an AO2 object.  It could just be an int.  This would require no ref-counting as the memory wouldn't be allocated.  Thread storage would get an allocation to stop a copy of the int, but that allocation would take care of itself.
> Initialization order:
> Everything uses the logger.  Unfortunately the 'exec_after_rotate' option creates a dummy channel to do variable substitution.  This pushes logging pretty far down the initialization list.  Many start-up messages are lost.  If the logger was initialized earlier it would require a way to block rotation until after it's safe to create a dummy channel for variable substitution.
> I'm not sure if it's reasonable to avoid using channels (trunk only).  This would involve having exec_after_rotation specifically replace $\{filename}.  We would also need to document that functions, expressions, and other variables are no longer supported.  Would be nice if the new procedure produced an error if unsupported expressions are detected.



--
This message was sent by Atlassian JIRA
(v6.2#6252)