Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Date: Sun, 17 Mar 2002 23:57:38 -0600 (CST)
Message-Id: <200203180557.g2I5vcG32904@newton.ch.collab.net>
From: Karl Fogel <kfogel@newton.ch.collab.net>
To: dev@apr.apache.org, dev@subversion.tigris.org
Subject: bug in apr_palloc()?
Reply-To: kfogel@collab.net
Emacs: the only text-editing software to require its own heat sink.

Is there maybe a bug in apr_palloc()?  I'm not 100% sure of it yet;
been trying to find the source of this bug in Subversion itself, so
far unsuccessfully.  Apologies that this reproduction recipe requires
building a patched Subversion; tried to come up with a smaller recipe,
but the bug didn't reproduce.

The symptoms below point to a bug either in apr_palloc(), or in SVN's
stringbuf allocating/appending/resizing code...

Environment:

   * Subversion revision 1538, built statically (./configure \
       --enable-maintainer-mode --disable-shared)

   * APR as of Mar 17 23:12:03 CST 2002

   * Debian GNU/Linux 2.4.14 #3 SMP Wed Nov 21 16:32:41 CST 2001 i686 unknown

Recipe:

First put this in your ~/.subversion/proxies file:

[somesection]
option1 = value1
option2 = value2
option3 = value3
optionX = valueX,

That is, it's a five-line file with no leading whitespace on any line,
nor trailing whitespace, and a newline after every line including the
last.  The comma on the end of the last line is crucial.  The file
size should be 83 bytes.

Next apply this patch to subversion/clients/cmdline/main.c:

Index: ./subversion/clients/cmdline/main.c
===================================================================
--- ./subversion/clients/cmdline/main.c
+++ ./subversion/clients/cmdline/main.c	Sun Mar 17 14:37:04 2002
@@ -34,6 +34,7 @@
 #include "svn_pools.h"
 #include "svn_wc.h"
 #include "svn_client.h"
+#include "svn_config.h"
 #include "svn_string.h"
 #include "svn_path.h"
 #include "svn_delta.h"
@@ -1056,6 +1057,26 @@
           return EXIT_FAILURE;
         }
     }
+
+  /* debugging code */
+  {
+    svn_config_t *cfg;
+    svn_string_t val;
+
+    svn_config_read_proxies (&cfg, pool);
+
+    svn_config_get (cfg, &val, "somesection", "option1", "NOT FOUND");
+    printf ("Found value: %s\n", val.data);
+
+    svn_config_get (cfg, &val, "somesection", "option2", "NOT FOUND");
+    printf ("Found value: %s\n", val.data);
+
+    svn_config_get (cfg, &val, "somesection", "option3", "NOT FOUND");
+    printf ("Found value: %s\n", val.data);
+
+    svn_config_get (cfg, &val, "somesection", "optionX", "NOT FOUND");
+    printf ("Found value: %s\n", val.data);
+  }
 
   err = (*subcommand->cmd_func) (os, &opt_state, pool);
   if (err)


Now rebuild, and run some Subversion command:

   floss$ subversion/clients/cmdline/svn st -nq
   Found value: value1
   Found value: value2
   Found value: value3
   Found value: valu v,

Notice how the last value is corrupted.  Let's trace that down:

In GDB, I set a breakpoint on libsvn_subr/config_file.c:parse_value(),
then skipped the first three breaks ("value1", "value2", "value3").
The fourth time, I started stepping through this loop in
parse_value():

  [...]
  /* Read the first line of the value */
  svn_stringbuf_setempty (ctx->value);
  for (ch = getc (ctx->fd); /* last ch seen was ':' or '=' in parse_option. */
       ch != EOF && ch != '\n';
       ch = getc (ctx->fd))
    {
      const char char_from_int = ch;
      svn_stringbuf_appendbytes (ctx->value, &char_from_int, 1);
    }
  /* Leading and trailing whitespace is ignored. */
  svn_stringbuf_strip_whitespace (ctx->value);
  [...]

If you let the loop finish, *ctx->value will already be corrupted by
the time we get to svn_stringbuf_strip_whitespace().  In fact, it's
corrupted right after we try to append the comma to the string.

So after reading the ',', step into svn_stringbuf_appendbytes().  Now
we're appending a comma to a stringbuf (date == " valueX") of length
7.  It's 7 since we haven't yet stripped the leading space, and the
terminating '\0' is not counted in svn_stringbuf_t's len, although it
must of course be accounted for in the allocated blocksize.

The stringbuf's blocksize is currently 8.  Note that the string is
full -- there's no room left, since the terminating '\0' occupies the
8th slot.  Thus, when svn_stringbuf_appendbytes calls
svn_stringbuf_ensure() with a requested length of 9 (to include the
terminating '\0'), svn_stringbuf_ensure() must call my__realloc().

Here's what GDB had to say in my__realloc():

   (gdb) s
   my__realloc (data=0x80fe94b " valueX", oldsize=7, request=16, 
                pool=0x80fe7d0) at subversion/libsvn_subr/svn_string.c:44
   (gdb) p data
   $4 = 0x80fe94b " valueX"
   (gdb) n  ### this ran the line "new_area = apr_palloc (pool, request);" ###
   (gdb) p new_area
   $5 = (void *) 0x80fe950
   (gdb) p (char *) new_area
   $6 = 0x80fe950 "eX"
   (gdb) n
   (gdb) p (char *) new_area
   $7 = 0x80fe950 " valu v"
   (gdb) c
   [... see corrupted output ...]
   (gdb) quit
   Debugger finished

Note how the `new_area' starts *inside* the old data, on the 'e'.
(0x80fe950 - 0x80fe94b) == 5, yet we had an original blocksize of 8.
The new block is supposed to be freshly allocated and non-overlapping
with any existing data, but it overlaps.  Ick! :-)

It's possible that the bug is in the stringbuf code -- that svn thinks
a certain amount is allocated for the original block, but in reality
less was allocated, so later when we apr_palloc(), the new block comes
up inside (what svn thinks of as "inside") the old block.

But if the bug is in the stringbuf code, I didn't find it tonight at
any rate.  I haven't traced into the APR code <hides head in shame,
half mocking, half real>, as others here know the APR pool code far
better than I.  If anyone wants to have a whack at this while I'm
asleep, please do!

-Karl