[asterisk-bugs] [JIRA] (ASTERISK-24833) [patch] audit of startup order reveals logger concerns

[Corey Farrell (JIRA)]

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

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

Further work on logger recursion avoidance to be posted against ASTERISK-24155.  The remaining issue brought up for this ticket is startup order.

I'm working on a patch that will add an 'int early' parameter to init_logger.  Early init would abort if exec_on_rotate is set.  This way the logger can start first thing on many systems and all testsuite runs.  The one concern is situations where logger.conf is changed to include exec_on_rotate after early logger init.  CLI commands can be delayed so they can't be used to reload, but SIGXFSZ can happen at any time.  It may be possible to defer responding to SIGXFSZ until the final init of logger.  If the SIGXFSZ was associated with a log file write error, that log channel would remain disabled at least until final init.

> [patch] audit of startup order reveals 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
>            Assignee: Corey Farrell
>            Severity: Minor
>         Attachments: callid-int-r2.patch, callid-int-r3.patch
>
>
> 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)