[Home]

Summary:ASTERISK-19451: va_start/va_copy and va_end do not always match up
Reporter:Walter Doekes (wdoekes)Labels:
Date Opened:2012-02-29 15:01:22.000-0600Date Closed:2013-02-25 06:49:18.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:General
Versions:SVN Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) ASTERISK-19451-1.8.diff
( 1) ASTERISK-19451-1.8--2.diff
Description:{noformat}
  va_end()
      Each invocation of va_start() must be matched by a corresponding invo‐
      cation of va_end() in the same function.  After  the  call  va_end(ap)
      the  variable  ap is undefined.  Multiple traversals of the list, each
      bracketed by va_start() and va_end() are possible.  va_end() may be  a
      macro or a function.
{noformat}
and
{quote}[...] on systems where arguments are passed in registers, it may be necessary for va_start() to allocate memory, store the arguments there, and also an indication of which argument is next, so that va_arg() can step through the list. Now va_end() can free the allocated memory again.{quote}

This is not always done right:

- there is a va_end too many in main/utils.c
- there are a couple too few in res/res_config_odbc.c and then lots of va_copy's aren't closed on early failure-return
- res/res_config_pgsql.c and res/res_config_curl.c have lots of va_ends but does not start/copy any (they shouldn't va_end, the callers of ast_load_realtime_helper handle both va_start/va_end)

And possibly a few more.
Comments:By: Walter Doekes (wdoekes) 2012-04-18 02:05:41.163-0500

{quote}
Apart from addons/res_config_mysql.c, I think you got them all.
{quote}

So yes.. (issue XYZ) but not (closes issue XYZ) ;)
By: Jonathan Rose (jrose) 2012-04-18 10:42:01.404-0500

Oh, sorry. I suppose it just finishes my role in the issue. I'll correct the svn log appropriately.
By: Matt Jordan (mjordan) 2013-02-24 17:20:34.629-0600

Patch attached for {{res_config_mysql.c}}.

It looks like it doesn't need to call va_end in all but a single case, since the configuration engine takes care of the calls for realtime providers.

Walter: does that look correct?
By: Walter Doekes (wdoekes) 2013-02-25 03:13:35.573-0600

Looks correct. But I did notice potential calls to va_arg after a SENTINEL had been encountered. I corrected those. (They can produce segfaults or possibly undefined behaviour in the rarest of cases.)

P.S.1. The value va_arg call is in most cases not checked for NULL. But if that fails, you get a clean segfault when accessing NULL and not random undefined behaviour. I left that as-is.

P.S.2. Even though config.h mandates that we pass SENTINEL around, the code using the va_list always checks against NULL instead. That's not nice, although it's probably never wrong.
By: Matt Jordan (mjordan) 2013-02-25 06:32:11.861-0600

Thanks, I missed the possible call to va_arg in the second assignment causing a problem when the first encountered a SENTINEL. I agree agree with both of your P.S.s - but those feel like they can be left either as an indication of a programming error or for improvements.