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
Versions:Frequency of
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"


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)



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)