[sac-dev] Bugs found

[Kuang He]

On Sat, Sep 6, 2008 at 2:06 AM, Kuang He <icrazy at gmail.com> wrote:
> I'm using SAC v101.1 on a linux box (Ubuntu 8.04), and the glibc
> version is 2.7 (2.7-10ubuntu3, to be exact).
>
> $ uname -a
> Linux ....... 2.6.24-19-generic #1 SMP Fri Jul 11 23:41:49 UTC 2008
> i686 GNU/Linux
> .....
> Bug 2: Putting a space after the comma in something like "&1,DIST"
> will _sometimes_ cause SAC to suddenly abort, with a message from
> glibc indicating possible double free. Below is an example of a case
> where this problem does not show up and another case where the problem
> does show up.
>
> $ sac
> SAC> r vel.sac
> SAC> evaluate to dist1 &1,dist
> SAC> message %dist1
>  2.84897$
> SAC> evaluate to dist1 &1, dist
> *** glibc detected *** /usr/local/sac/bin/sac: double free or
> corruption (!prev): 0x0843f020 ***
> ======= Backtrace: =========
> /lib/tls/i686/cmov/libc.so.6[0xb7c9ba85]
> /lib/tls/i686/cmov/libc.so.6(cfree+0x90)[0xb7c9f4f0]
> ....

With Brian's help, I was able to locate the place in the source code
that caused this problem (his OSX machines don't have this problem):
line 93 of src/cpf/cfmt.c . The code there does not have double
free()'s, but code snippets shown below just do not make any sense to
me: temporal variable strtemp1 gets created and destroyed, without
doing anything useful at all. Commenting out these does solve the
problem.

$ diff -u src/cpf/cfmt.c.old src/cpf/cfmt.c
--- src/cpf/cfmt.c.old  2008-09-08 00:07:33.000000000 -0400
+++ src/cpf/cfmt.c      2008-09-08 01:48:27.000000000 -0400
@@ -86,11 +86,14 @@
                        iend_ = ibeg + nchar + 2;
                        if( iend_ <= MCMSG ){
                                kmsg[ibeg - 1] = kmcom.kcom[j - 1][0];
+                               /*
                                 strtemp1 = malloc(MCMSG+1-(ibeg+1));
                                 strncpy(strtemp1,kmsg+ibeg,MCMSG+1-(ibeg+1));
                                 strtemp1[MCMSG+1-(ibeg+1)] = '\0';
                                copykc( (char*)kmcom.kcom[j + 1],9,
nchar, strtemp1);
                                 free(strtemp1);
+                               */
                                kmsg[iend_ - 2] = kmcom.kcom[j - 1][0];
                                kmsg[iend_ - 1] = ' ';
                                if( j == cmcom.jcom )
@@ -103,11 +106,13 @@
                        nchar = (long)( Flnum[j + 1] + 0.1 );
                        iend_ = ibeg + nchar;
                        if( iend_ <= MCMSG ){
+                               /*
                                 strtemp1 = malloc(MCMSG+1-ibeg);
                                 strncpy(strtemp1,kmsg+ibeg-1,MCMSG+1-ibeg);
                                 strtemp1[MCMSG+1-ibeg] = '\0';
                                copykc( (char*)kmcom.kcom[j + 1],9,
nchar, strtemp1);
                                 free(strtemp1);
+                               */
                                kmsg[iend_ - 1] = ' ';
                                if( j == cmcom.jcom )
                                        iarrow = ibeg - ndiff;


By the way, I think wrapping all the uses of free() to FREE() shown
below would be a good idea. The catch is just that since the code base
is too big, it'll take quite some time to change all of them.

#define FREE(ptr)   do { if (ptr) free(ptr); } while (0)


Best regards,

-- 
Kuang He
Department of Physics
University of Connecticut
Storrs, CT 06269-3046

Tel: +1.860.486.4919
Web: http://www.phys.uconn.edu/~he/