[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:all-tabpanel ]

Corey Farrell updated ASTERISK-24833:
-------------------------------------

    Attachment: ast_log-early-init.patch

This is what I've come up with for a patch against master to initialize logging as early as it can.  Early initialization is canceled (default console logging enabled) if exec_after_rotate is used.  A new option post_rotate_exec is created which should use bash shell variable replacement for $\{filename\}.

[~mjordan]: I have not yet has a chance to test this well (been busy with another change), but I wanted to post it here see if you think it's a reasonable plan.  Should we go with a better variable name for post_rotate_exec?  Maybe AST_LOG_FILENAME or something?

> [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: ast_log-early-init.patch, 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)