[Home]

Summary:ASTERISK-18109: Segfault in shell_helper in func_shell.c
Reporter:Michael Myles (xdrive)Labels:
Date Opened:2011-07-11 08:55:10Date Closed:2011-08-11 16:47:03
Priority:MinorRegression?
Status:Closed/CompleteComponents:Functions/func_shell
Versions:1.8.4 Frequency of
Occurrence		Occasional
Related
Issues:
must be merged before resolvingASTERISK-18293 Merge: ASTERISK 18109
Environment:Attachments:
Description:Asterisk segfaults with a call to fgets() in shell_helper function.
File pointer is null with no check against this

Program terminated with signal 11, Segmentation fault.

#0  0x000000366fc614dd in fgets () from /lib64/libc.so.6
(gdb) bt
#0  0x000000366fc614dd in fgets () from /lib64/libc.so.6
#1  0x00002aaab85d2acc in shell_helper (chan=0xce51b08, cmd=<value optimized out>, data=<value optimized out>, buf=0x453c6810 "", len=4096) at func_shell.c:54
#2  0x00000000004ed377 in ast_func_read (chan=0xce51b08, function=<value optimized out>, workspace=0x453c6810 "", len=4096) at pbx.c:3498
#3  0x00000000004f04a5 in pbx_substitute_variables_helper_full (c=0xce51b08, headp=0xce51ec0, cp1=<value optimized out>, cp2=0x453cb996 "", count=8185, used=0x453cde58) at pbx.c:3879
(gdb) frame 1
#1  0x00002aaab85d2acc in shell_helper (chan=0xce51b08, cmd=<value optimized out>, data=<value optimized out>, buf=0x453c6810 "", len=4096) at func_shell.c:54
54 while (fgets(plbuff, sizeof(plbuff), ptr)) {
(gdb) p ptr
$1 = (FILE *) 0x0

Not sure how it gets in this state, but a fix/workaround would be to check and make sure the file pointer is not null before entering the while loop.


53a54,57
> if (!ptr) {
> ast_log(LOG_ERROR, "Null pointer in shell_helper. fgets would fail!\n");
> return -1;
> }

Comments:By: Leif Madsen (lmadsen) 2011-07-11 15:45:03.580-0500

Thank you for your bug report. In order to move your issue forward, we require a backtrace[1] from the core file produced after the crash. Also, be sure you have DONT_OPTIMIZE enabled in menuselect within the Compiler Flags section, then:

make install

After enabling, reproduce the crash, and then execute the backtrace[1] instructions. When complete, attach that file to this issue report.

[1] https://wiki.asterisk.org/wiki/display/AST/Getting+a+Backtrace

*** Problem with the backtrace provided is that it is optimized ***
By: Michael Myles (xdrive) 2011-07-11 16:25:13.727-0500

This problem should be completely evident regardless of backtrace output.
In the original code, there is NO check to make sure that popen() actually succeeds in creating a pipe to execute a shell command.


static int shell_helper(struct ast_channel *chan, const char *cmd, char *data,
                        char *buf, size_t len)
{
if (ast_strlen_zero(data)) {
ast_log(LOG_WARNING, "Missing Argument!  Example:  Set(foo=${SHELL(echo \"bar\")})\n");
return -1;
}

if (chan)
ast_autoservice_start(chan);

if (len >= 1) {
FILE *ptr;
char plbuff[4096];

ptr = popen(data, "r");
while (fgets(plbuff, sizeof(plbuff), ptr)) {
strncat(buf, plbuff, len - strlen(buf) - 1);
}
pclose(ptr);
}

if (chan)
ast_autoservice_stop(chan);

return 0;
}

Directly from the man page on popen:

RETURN VALUE
      The popen() function returns NULL if the fork(2) or pipe(2) calls fail, or if it cannot allocate memory.

The code above makes no assumption that the return value may indeed be NULL.

By: Walter Doekes (wdoekes) 2011-07-21 14:10:14.962-0500

Agreed. Return values should be checked.

But, popen shouldn't be failing in the first place. Are you running out of fd's perhaps? (ulimit -n)
By: Michael Myles (xdrive) 2011-07-21 15:05:09.869-0500

Agreed it shouldn't, but that doesn't mean it can't.
You may be right, that could very well be the case, this user's limit is 1024.

lsof | grep -P "asterisk.*(REG|FIFO)" | wc -l

gives me roughly half that number open in regular files and pipes at any given time.

I'm not typically looking at the machine while asterisk is crashing and restarting, so I can't say for sure if that's what is causing it to fail.
It happens roughly once every day or so since we upgraded from 1.8.1 to 1.8.4 (which is not all that frequent so it's hard to catch)

I can, however, say that just checking to make sure it's got a valid fd pointer does keeps it from segfaulting when this does occur.
By: Michael Myles (xdrive) 2011-08-01 10:59:50.135-0500

This should likely also stop auto servicing the channel in this condition.

if (!ptr) {
   ast_log(LOG_ERROR, "Null pointer in shell_helper. fgets would fail!\n");
   if (chan)
       ast_autoservice_stop(chan);
   return -1;
}
By: Richard Mudgett (rmudgett) 2011-08-11 16:45:51.280-0500

Committed trunk -r331577.
Merged revisions 331576 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/10

................
 r331576 | rmudgett | 2011-08-11 16:42:21 -0500 (Thu, 11 Aug 2011) | 16 lines

 Merged revisions 331575 via svnmerge from
 https://origsvn.digium.com/svn/asterisk/branches/1.8

 ........
   r331575 | rmudgett | 2011-08-11 16:39:58 -0500 (Thu, 11 Aug 2011) | 9 lines

   Segfault in shell_helper in func_shell.c.

   The return value of popen() was not checked for failure to open.

   (closes issue ASTERISK-18109)
   JIRA SWP-3633
   Reported by: Michael Myles
   Tested by: rmudgett
 ........
................