Bug 127639 - Segfault in x_realloc devel/ccache
Segfault in x_realloc devel/ccache
Status: Closed FIXED
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s)
Latest
Any Any
: Normal Affects Only Me
Assigned To: ahze
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-25 20:50 UTC by mel.xyzzy
Modified: 2008-11-24 03:40 UTC (History)
1 user (show)

See Also:


Attachments
patch-args.c (516 bytes, text/plain)
2008-09-25 20:50 UTC, mel.xyzzy
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description mel.xyzzy 2008-09-25 20:50:02 UTC
util.c:
   184    this is like realloc() but dies if the malloc fails
   185  */
   186  void *x_realloc(void *ptr, size_t size)
   187  {
   188          void *p2;
   189          if (!ptr) return x_malloc(size);
   190          p2 = malloc(size);
   191          if (!p2) {
   192                  fatal("out of memory in x_realloc");
   193          }
   194          if (ptr) {
   195                  memcpy(p2, ptr, size);
   196                  free(ptr);
   197          }
   198          return p2;
   199  }

args.c:
    38  void args_add(ARGS *args, const char *s)
    39  {
    40          args->argv = (char**)x_realloc(args->argv, (args->argc + 2) * sizeof(char *));
    41          args->argv[args->argc] = x_strdup(s);
    42          args->argc++;
    43          args->argv[args->argc] = NULL;
    44  }

Line 195 copies newsize of oldpointer to new pointer which can produce the following backtrace:
(gdb) bt
#0  0x0000000800816b86 in memcpy () from /lib/libc.so.6
#1  0x0000000000403fec in x_realloc (ptr=0x514800, size=2056) at util.c:195
#2  0x0000000000404512 in args_add (args=0x512040,
    s=0x7fffffffe2c3 "p12_key.So") at args.c:40
#3  0x00000000004045a1 in args_init (init_argc=455, init_args=0x7fffffffcf20)
    at args.c:32
#4  0x0000000000402a14 in main (argc=455, argv=0x7fffffffc720) at ccache.c:564

Fix: The following works around the problem by using reallocf, instead of
x_malloc, however, the root of the problem is likely elsewhere.
How-To-Repeat: I can't reproduce this using a test like this:
ln -s ccache cc
./cc -L/usr/lib -shared `jot -w 'file%04u.So' 452 1 452`

However, the following reproduces the bug reliably:
#!/bin/sh

SRCDIR=${SRCDIR:="/usr/src"}

cd ${SRCDIR}/secure/lib/libcrypto
rm -f `make -V .OBJDIR`/libcrypto.so.4
cd ${SRCDIR}
make everything
Comment 1 Edwin Groothuis freebsd_committer 2008-09-25 20:50:11 UTC
Responsible Changed
From-To: freebsd-ports-bugs->ahze

Over to maintainer (via the GNATS Auto Assign Tool)
Comment 2 tg 2008-10-05 18:10:49 UTC
Hello all,

I found and fixed a rather interesting bug in ccache=E2=80=99s x_realloc()
function, which tries to read out too many bytes from the _old_
storage after allocating the new storage. The obvious fix is to
make it use realloc(3) instead.

Interestingly enough, there is an =E2=80=9Cif (ptr)=E2=80=9D in the (origin=
al) code
which is always true, since the =E2=80=9Cif (!ptr)=E2=80=9D case was alread=
y handled=E2=80=A6
seems as if the author of that code did not know what he was doing.

Effects of the bug:
=E2=80=A2 confirmed: segfault abortion (DoS)
=E2=80=A2 don=E2=80=99t know if it can lead to data corruption, don=E2=80=
=99t think so

Fix: http://www.mirbsd.org/cvs.cgi/ports/devel/ccache/patches/patch-util_c

bye,
//mirabilos, who first thought his memcpy(3) was broken=E2=80=A6
--=20
Sometimes they [people] care too much: pretty printers [and syntax highligh=
-
ting, d.A.] mechanically produce pretty output that accentuates irrelevant
detail in the program, which is as sensible as putting all the prepositions
in English text in bold font.=09-- Rob Pike in "Notes on Programming in C"
Comment 3 mel.xyzzy 2008-10-05 20:04:19 UTC
On Sunday 05 October 2008 19:10:49 Thorsten Glaser wrote:
> Hello all,
>
> I found and fixed a rather interesting bug in ccache=E2=80=99s x_realloc()
> function, which tries to read out too many bytes from the _old_
> storage after allocating the new storage. The obvious fix is to
> make it use realloc(3) instead.
>
> Interestingly enough, there is an =E2=80=9Cif (ptr)=E2=80=9D in the (orig=
inal) code
> which is always true, since the =E2=80=9Cif (!ptr)=E2=80=9D case was alre=
ady handled=E2=80=A6
> seems as if the author of that code did not know what he was doing.
>
> Effects of the bug:
> =E2=80=A2 confirmed: segfault abortion (DoS)
> =E2=80=A2 don=E2=80=99t know if it can lead to data corruption, don=E2=80=
=99t think so
>
> Fix: http://www.mirbsd.org/cvs.cgi/ports/devel/ccache/patches/patch-util_c
>
> bye,
> //mirabilos, who first thought his memcpy(3) was broken=E2=80=A6

I think the intent of x_realloc is to not rely on realloc being available o=
n=20
target OS. If you're going to use realloc anyway, then why not #if 0=20
x_realloc and compile with  -Dx_realloc=3Drealloc.

@upstream:
Thing is, userspace realloc cannot determine sizeof(old), without digging i=
nto=20
target OS's allocation algorithm (i.e. what  lib/libc/stdlib/malloc.c:irall=
oc=20
does on FreeBSD), so x_realloc should really be prototyped as
void *x_realloc(void *old, size_t old, size_t new);

and act accordingly.
=2D-=20
Mel
Comment 4 tg 2008-10-05 21:48:27 UTC
Mel dixit:

>I think the intent of x_realloc is to not rely on realloc being available =
on=20
>target OS.

What OS does not have realloc? oO

>@upstream:
>Thing is, userspace realloc cannot determine sizeof(old), without digging =
into=20
>target OS's allocation algorithm (i.e. what  lib/libc/stdlib/malloc.c:iral=
loc=20
>does on FreeBSD), so x_realloc should really be prototyped as
>void *x_realloc(void *old, size_t old, size_t new);

Yes, indeed.

bye,
//mirabilos
--=20
[...] if maybe ext3fs wasn't a better pick, or jfs, or maybe reiserfs, oh b=
ut
what about xfs, and if only i had waited until reiser4 was ready... in the =
be-
ginning, there was ffs, and in the middle, there was ffs, and at the end, t=
here
was still ffs, and the sys admins knew it was good. :)  -- Ted Unangst =C3=
=BCber *fs
Comment 5 ahze freebsd_committer 2008-11-24 03:22:35 UTC
State Changed
From-To: open->closed

Committed, Thanks!
Comment 6 dfilter freebsd_committer 2008-11-24 03:32:59 UTC
ahze        2008-11-24 03:32:46 UTC

  FreeBSD ports repository

  Modified files:
    devel/ccache         Makefile 
  Added files:
    devel/ccache/files   patch-util.c 
  Log:
  Fix a bug in x_realloc() which tries to read out too many bytes from the old storage after allocating new storage.
  
  PR:             ports/127639
  Submitted by:   Thorsten Glaser <tg@mirbsd.de> (MirBSD project)
  Reported by:    Mel <mel.xyzzy@rachie.is-a-geek.net>
  
  Revision  Changes    Path
  1.44      +1 -1      ports/devel/ccache/Makefile
  1.1       +25 -0     ports/devel/ccache/files/patch-util.c (new)
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"