[Home]

Summary:ASTERISK-01985: [patches][src-audit] cli.c, dlfcn.c bufferoverrun, malloc checks
Reporter:Rob Gagnon (rgagnon)Labels:
Date Opened:2004-07-10 02:03:56Date Closed:2008-01-15 15:02:23.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) cli.c.patch.txt
( 1) cli.c.patch.txt
( 2) dlfcn.c.patch.txt
( 3) enum.c.patch.txt
( 4) file.c.patch.txt
( 5) logger.c.patch.txt
( 6) manager.c.patch.txt
( 7) muted.c.patch.txt
( 8) pbx.c.patch.txt
( 9) srv.c.patch.txt
(10) translate.c.patch.txt
Description:Different version of patch for cli.c than in ASTERISK-1972004.  This one uses a small re-usable buffer to snprintf(), and then uses strncat() to add to the main buffer.

dlfcn.c:  strcpy, etc. changes, but mostly other changes where result of malloc() was not checked before pointer being used.

I think I might try doing sets of *.c files for each letter of the alphabet so I can keep track where I leave off.  So these couple of files complete "c" and "d" before going into sub directories

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

[disclaimed]

I cannot compile dlfcn.c on my machine.  Someone with the appropriate architecture might want to test this.  I had already made the code changes by the time I noticed it would not be included in OBJS for my machine.
Comments:By: zoa (zoa) 2004-07-10 03:12:52

thanks a lot for you contributions, rgagnon, its amazing !
By: Rob Gagnon (rgagnon) 2004-07-10 04:35:57

Thanks zoa.  I have a script that is helping me to find possible issues.  The script DOES NOT do any code changes.  It just helps find things like strncpy() commands that appear to not have the "-1" in the line, etc.  Then I can go look at it to see if it needs touch-up.

Also, I extended this group of patches up to the letter "t" (except I skipped "say.c"...  I started translating the Polish code in there, and I don't want to put it up incomplete)

One note about the patch for pbx.c... The changes to ast_device_state_changed() are minor, but look major in the patch.  I reformatted it a bit.

edited on: 07-10-04 04:24
By: Mark Spencer (markster) 2004-07-12 22:13:40

I'd still like to see the cli.c.patch.txt improved.  The first was much more readable.  Perhaps you can just use snprintf and then strlen to get the length and += that instead.
By: Rob Gagnon (rgagnon) 2004-07-13 01:36:50

So something like this?:

 snprintf(timestr + pos, maxbytes, "%d years, ", years);
 bytes = strlen(timestr + pos);
 pos += bytes;
 maxbytes -= bytes;

It would make for 4 lines for each snprint() in the function.
By: Rob Gagnon (rgagnon) 2004-07-13 14:43:20

Check out a new cli.c.patch.txt file  (4,539 bytes) 07-13-04 14:41

This one optimizes the values being 1 or >1 as well by using a macro called "ESS"  as well as going back to the offset+=, maxbytes-= method.
By: Mark Spencer (markster) 2004-07-14 03:58:43

Okay I've merged all these, except dlfcn.  Without someone to test it, and given the relative complexity of the changes there this late in the game, i'm inclined to leave it alone unless we get someone to okay it.
By: Digium Subversion (svnbot) 2008-01-15 15:02:23.000-0600

Repository: asterisk
Revision: 3430

U   trunk/cli.c
U   trunk/enum.c
U   trunk/file.c
U   trunk/logger.c
U   trunk/manager.c
U   trunk/muted.c
U   trunk/pbx.c
U   trunk/srv.c
U   trunk/translate.c

------------------------------------------------------------------------
r3430 | markster | 2008-01-15 15:02:22 -0600 (Tue, 15 Jan 2008) | 2 lines

Remaining rgagnon source audit improvements (bug ASTERISK-1985)

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

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