[Home]

Summary:ASTERISK-01952: [patch] app_voicemail.c bad strncpy() and snprintf() call
Reporter:Rob Gagnon (rgagnon)Labels:
Date Opened:2004-07-06 02:52:08Date Closed:2008-01-15 15:01:56.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) app_voicemail.c.patch.txt
( 1) app_voicemail.c.patch.txt
Description:A strncpy() call is missing the "-1" and a snprintf() right below it has an un-needed "-1"

****** ADDITIONAL INFORMATION ******

[disclaimed]
Comments:By: Tilghman Lesher (tilghman) 2004-07-06 13:06:28

Leads to another question.  Why is there a strncpy there in the first place, when the subsequent statement overwrites the destination?  Why not just remove the strncpy?
By: Rob Gagnon (rgagnon) 2004-07-06 13:09:59

LOL.  good point.  let me tweak that
By: Rob Gagnon (rgagnon) 2004-07-06 13:18:53

Corydon, The 839 byte patch solves that, as well as replaces the check for empty string with ast_strlen_zero()
By: Rob Gagnon (rgagnon) 2004-07-07 04:30:05

Major change to patch.  You may want to change status of this bug.  Here's why:

From the -dev mailing list, someone noted a bad-looking snprintf() along these lines in app_voicemail.c:

static int vm_browse_messages(struct ast_channel *chan, struct vm_state *vms,  struct ast_vm_user *vmu, int lastmsg, int curmsg, char *fn, char *curbox)
{
...
    snprintf(fn, sizeof(fn) + sizeof(curbox) + 2, "vm-%s", curbox);
...
}


As you can see, this is a place for a nice buffer overrun problem, since the sizeof(fn) and sizeof(curbox) are 4 each (on my platform anyhow, and most other 32 bit machines)

Sooooo.... I decided to have a little look-see at ALL of the snprintf() calls.  I found a couple more exactly like this.

Then I found the prototype to the function (and its 2 brothers):

static int vm_browse_messages(struct ast_channel *chan, struct vm_state *vms, struct ast_vm_user *vmu, int lastmsg, int curmsg, char *fn, char *curbox)

is way long, and can be changed to:

static int vm_browse_messages(struct ast_channel *chan, struct vm_state *vms, struct ast_vm_user *vmu)

since all calls to the function use members of "vms" for all the parameters following "vmu")

This then led me to a lot of the adsi_xxx functions (adsi_status, adsi_status2, adsi_delete, etc.)

All of these functions had prototypes way longer than they need to be, so I modified them to accept the shorter version.

Continuing on, this should now become a  "source-audit, repair-of-possible-buffer-overrun, cleanup-of-long-prototypes" kind of bug.

I think the patch should be easy to read and see that I changed no functionality at all, except to stablize the code, and make it easier to read/repair.
By: Mark Spencer (markster) 2004-07-07 08:44:03

Fixed in CVS.  Very nicely done!
By: Digium Subversion (svnbot) 2008-01-15 15:01:44.000-0600

Repository: asterisk
Revision: 3383

U   trunk/apps/app_voicemail.c

------------------------------------------------------------------------
r3383 | markster | 2008-01-15 15:01:43 -0600 (Tue, 15 Jan 2008) | 2 lines

voicemail cleanups from rgagnon (bug ASTERISK-1952)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=3383
By: Digium Subversion (svnbot) 2008-01-15 15:01:56.000-0600

Repository: asterisk
Revision: 3398

U   trunk/apps/app_voicemail.c

------------------------------------------------------------------------
r3398 | markster | 2008-01-15 15:01:56 -0600 (Tue, 15 Jan 2008) | 2 lines

Re-fix bug ASTERISK-1952 (bug ASTERISK-1971)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=3398