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

[Corey Farrell (JIRA)]

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

Corey Farrell commented on ASTERISK-24833:
------------------------------------------

Though I've flagged this as an issue for 11 and 13, most of I've proposed is inappropriate for already released branches.  Most of this discussion is ideas of what can be done in trunk.  I'm OK with saying "it is what it is" about 11 and 13.

{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}
This leads me to believe that replacing code that uses 'struct ast_str' with standard C string routines and removing ao2 from callid's would be the better fix.

{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 was not suggesting that logger init should happen before fork, but it should happen as soon as possible after fork.  There are some other inits that have to happen first (in case we have a DEBUG compile option on).  The idea is to minimize the stuff we do before initializing the logger, that way we minimize the number of messages that are lost.

{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.
{quote}
It might be best to defer {{rotate_file}} and logger reloads until after fully booted.  If it's called too early save to a list of files to rotate after fully booted.  Rotation would be done after all modules are loaded, since for example {{pbx_config}} could initialize a global variable for use by {{exec_after_rotate}}.  It could be appropriate to look at this for 11 and 13.

> 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)