patch: under Windows, cvs status reports "memory exhausted"
Chris Bohn
This is also tracked in issue 168:
http://ccvs.cvshome.org/issues/show_bug.cgi?id=168
I wanted to submit the patch to this list as well because that is the
way suggested in HACKING. See the issue for full details, but the patch
is also pasted below. I really think this should be in the next release
since Windows users can't run correctly without it. The root problem is
in subr.c, but the problem affects nearly all subcommands because the
problem code is hit in send_file_names, which is called for nearly
everything.
There is a whole testing framework for unix, but there is nothing for
Windows. I think if there was any for Windows, this would have been
caught because doing a simple cvs status with a file name argument
exposes the problem. I realize many of the developers of cvs do their
work on unix, but if Windows is going to be supported, then some type of
regression testing framework should probably be made for it too,
especially now with different code paths because of the case sensitivity
changes (where this bug was located).
Chris
NEWS entry:
* 'cvs status' now works on Windows and no longer gives a memory
exhausted error (under client/server mode)
Log entry:
2004-03-24 Chris Bohn <cb...@rrinc.com>
* subr.c (expand_string): fixed to have new implementation match more
that of the old implementation (realloc to newsize, set current size
passed in to the new size)
[D:\temp\ccvs\src]cvs diff -r cvs1-12-6 subr.c
Index: subr.c
===================================================================
RCS file: /cvs/ccvs/src/subr.c,v
retrieving revision 1.103
diff -r1.103 subr.c
36,37c36,40
< while (*n < newsize)
< *strptr = x2realloc (*strptr, n);
---
> if (*n < newsize)
> {
> *strptr = x2realloc (*strptr, &newsize);
> *n = newsize;
> }
Derek Robert Price
Hash: SHA1
Chris Bohn wrote:
> in cvs 1.12.6 client/server mode
>
> This is also tracked in issue 168:
> http://ccvs.cvshome.org/issues/show_bug.cgi?id=168
>
> I wanted to submit the patch to this list as well because that is the
> way suggested in HACKING. See the issue for full details, but the
> patch is also pasted below. I really think this should be in the next
> release since Windows users can't run correctly without it. The root
> problem is in subr.c, but the problem affects nearly all subcommands
> because the problem code is hit in send_file_names, which is called
> for nearly everything.
I'm not sure what is causing your problem on Windows, but I doubt it is
this. x2realloc(str, &size) is guaranteed to increase SIZE with each
call. This particular implementation doubles it and there should be no
differences under Windows.
Can you supply a stack trace for your failure case?
> There is a whole testing framework for unix, but there is nothing for
> Windows. I think if there was any for Windows, this would have been
> caught because doing a simple cvs status with a file name argument
> exposes the problem. I realize many of the developers of cvs do their
> work on unix, but if Windows is going to be supported, then some type
> of regression testing framework should probably be made for it too,
> especially now with different code paths because of the case
> sensitivity changes (where this bug was located).
Yep. This would be nice. I've thought so for awhile.
Derek
- --
*8^)
Email: de...@ximbiot.com
Get CVS support at <http://ximbiot.com>!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)
Comment: Using GnuPG with Netscape - http://enigmail.mozdev.org
iD8DBQFAYzzvLD1OTBfyMaQRAhA8AJwNLtEXs8yg5t+8bq+2/64bNCs7dQCgxxpV
eza+QFmOGCPMyWwW4egkGPw=
=dXCe
-----END PGP SIGNATURE-----
Chris Bohn
expand_string, which is called from xrealloc_and_strcat.
You can look at the diff of the subr.c code that changed in cvs 1.12.6
to see the differences:
revision 1.100
date: 2004/02/21 00:09:38; author: dprice; state: Exp; lines: +2 -33
* subr.c (expand_string): Use x2realloc() from GNULIB for core
functionality.
----------------------------
revision 1.99
date: 2004/02/20 23:16:55; author: dprice; state: Exp; lines: +0 -24
Merge changes from 1.11.x.
client.c does like this under Windows (in send_file_names):
/* Add the slash unless this is our first element. */
if (line_len)
xrealloc_and_strcat (&line, &line_len, "/");
xrealloc_and_strcat (&line, &line_len, node->key);
and later
/* If node is still NULL then we either didn't find CVSADM or
* we didn't find an entry there.
*/
if (node == NULL)
{
/* Add the slash unless this is our first element. */
if (line_len)
xrealloc_and_strcat (&line, &line_len, "/");
xrealloc_and_strcat (&line, &line_len, q);
break;
}
and
/* Now put everything we didn't find entries for back on. */
while (q = pop (stack))
{
if (line_len)
xrealloc_and_strcat (&line, &line_len, "/");
xrealloc_and_strcat (&line, &line_len, q);
free (q);
}
Notice how it is relying on the passed in line_len parameter to be set
to the new size when the function is done.
xrealloc_and_strcat looks like this in both versions of the file:
void
xrealloc_and_strcat (char **str, size_t *lenp, const char *src)
{
expand_string (str, lenp, strlen (*str) + strlen (src) + 1);
strcat (*str, src);
}
Remember the second parameter, lenp (line_len in the actual function
call in client.c), is what client.c is expecting to be changed to be the
new size.
In 1.99 of subr.c, expand_string looks like:
void
expand_string (char **strptr, size_t *n, size_t newsize)
{
if (*n < newsize)
{
while (*n < newsize)
{
if (*n < MIN_INCR)
*n = MIN_INCR;
else if (*n >= MAX_INCR)
*n += MAX_INCR;
else
{
*n *= 2;
if (*n > MAX_INCR)
*n = MAX_INCR;
}
}
*strptr = xrealloc (*strptr, *n);
}
}
So it is the second parameter here too, n, that needs to be reset to the
new size for everything to work as before. In this version of the
function, *n is what is set to higher values until it is bigger than the
requested new size, so when the function is done, that parameter is
modified to be the new, ending size of the string.
In 1.100 of subr.c, first released with cvs 1.12.6, expand_string looks
like:
void
expand_string (char **strptr, size_t *n, size_t newsize)
{
while (*n < newsize)
*strptr = x2realloc (*strptr, n);
}
The first line will likely always be true because this function is
called to append two strings in this case. x2realloc is called passing
in only the old length, which is initially 0 when passed down from
client.c (send_file_names) in the first call. 0 will always be <
newsize, which is a calculation using the real string lengths of the
strings in question. This means the while loop will never terminate
because *n will always be less than newsize because *n is never changing
anywhere in the new implementation. When a non-zero n is passed in,
x2realloc will keep reallocating until it runs out of memory. My
suggested fix makes the function implementation look like:
void
expand_string (char **strptr, size_t *n, size_t
newsize)
{
if (*n < newsize)
{
*strptr = x2realloc (*strptr, &newsize);
*n = newsize;
}
}
which makes the "while" an "if" just in case (and there is no reason to
loop now using x2realloc), sends in newsize to x2realloc since that is
the new size desired, and then updates the second parameter, *n, to be
the newly reallocated size, which is what the calling functions were
expecting. As I indicated in the issue (not the email), this is only a
problem under Windows because the code that causes the problem in
client.c (calls to xrealloc_and_strcat) is in an #ifdef
FILENAMES_CASE_INSENSITIVE block, and the same code isn't run on unix.
So I think I have everything correct.
Chris
Derek Robert Price
Hash: SHA1
Chris Bohn wrote:
> x2realloc wasn't the problem; it was the wrapper function around it,
> expand_string, which is called from xrealloc_and_strcat.
x2realloc (str, size) doubles *size (setting it in the process), and
returns xrealloc (str, *size). This is exactly what expand_string()
used to be doing.
There should also be no difference in the behavior of expand_string() or
x2realloc() between UNIX & Windows. Can you provide a debugger stack
trace for the `cvs status' failure you reported?
Once again:
> void
> expand_string (char **strptr, size_t *n, size_t newsize)
> {
> while (*n < newsize)
> *strptr = x2realloc (*strptr, n);
> }
>
> The first line will likely always be true because this function is
> called to append two strings in this case.
No, it won't. In your example above, x2realloc(*strptr, n) doubles *n
with each invocation. Please provide a stack trace.
Derek
- --
*8^)
Email: de...@ximbiot.com
Get CVS support at <http://ximbiot.com>!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)
Comment: Using GnuPG with Netscape - http://enigmail.mozdev.org
iD8DBQFAY0kNLD1OTBfyMaQRAjpXAJ4qnRzoDgLnNVAHO5kxRee9Zz4PHwCfaQ8s
92LRHhNFESk5mUApftthq2k=
=d9f4
-----END PGP SIGNATURE-----
Chris Bohn
cvs -d :pserver:admiral:/zip/tmp/cbohn/cvstest stat subr.c
from within the cvs src directory.
On the first invocation of xrealloc_and_strcat
xrealloc_and_strcat (&line, &line_len, node->key);
line 4318 of client.c (cvs 1.12.6 source)
line = ""
line_len = 0
node->key = subr.c
That calls causes the error.
Upon getting into expand_string from xrealloc_and_strcat:
expand_string (char **strptr, size_t *n, size_t newsize)
*strptr = ""
*n = 0
newsize = 7 (calculated in xrealloc_and_strcat from strlen (*str) +
strlen (src) + 1 --> 0 + 6 + 1)
So we now have:
while (*n < newsize)
being
while (0 < 7)
in the full function:
void
expand_string (char **strptr, size_t *n, size_t newsize)
{
while (*n < newsize)
*strptr = x2realloc (*strptr, n);
}
The x2realloc call is the one that dies.
x2realloc calls x2nrealloc_inline (p, pn, 1);
being logically equivalent to x2nrealloc_inline ("", 0, 1);
Here is that function:
static inline void *
x2nrealloc_inline (void *p, size_t *pn, size_t s)
{
size_t n = *pn;
if (! p)
{
if (! n)
{
/* The approximate size to use for initial small allocation
requests, when the invoking code specifies an old size of
zero. 64 bytes is the largest "small" request for the
GNU C library malloc. */
enum { DEFAULT_MXFAST = 64 };
n = DEFAULT_MXFAST / s;
n += !n;
}
}
else
{
if (SIZE_MAX / 2 / s < n)
xalloc_die ();
n *= 2;
}
*pn = n;
return xrealloc (p, n * s);
}
removing inline so I could debug into in the IDE, it revealed it hits
the else of "if (! p)" because p was allocated back in client.c:
char *line = xmalloc (1);
*line = '\0';
so in this case, the if above the xalloc_die isn't called because the
expr is > 0, so n and *pn get set to 0*2 = 0 still.
xrealloc (p, n * s) gets called with "", 0*1 --> xrealloc ("", 0)
that calls
xnrealloc_inline (p, n, 1);
and xalloc_die is called from
if (xalloc_oversized (n, s) || ! (p = realloc (p, n * s)))
xalloc_oversized is false, but since realloc is called like
realloc(p, 0), it reallocs the block to 0, and !0 = 1, so it dies. I
thought I would step through the runtime a little more, and I found:
/*
* ANSI: realloc(pUserData, 0) is equivalent to free(pUserData)
* (except that NULL is returned)
*/
if (fRealloc && nNewSize == 0)
{
_free_dbg(pUserData, nBlockUse);
return NULL;
}
so that explains the problem. I apologize for not digging in this much
initially, but when initially debugging, the debugger wasn't going where
I expected because of the inline calls, so I just thought something was
messed up and ignored the problem. So the solution to this problem is
debatable. Should client.c not xmalloc(1), should x2nrealloc_inline be
smarter and not just check !p, etc.? Changing x2nrealloc_inline sounds
like the best way to me. You could do something like checking for a
zero len string and size allocated only being 1, but you have to cast
everything from the void *, which likely isn't desirable. I can't
really find any docs on what these x2 functions are supposed to do
(exact specs), so I don't know if they should change or if the caller
(client.c) should have known the proper usage. Either way, it is a bad
error coming out of x2realloc, so that should change at a minimum to
return a better error if 0 isn't valid with an already allocated string.
My fix will still work, but it is allocating more than it needs in the
case of already allocated string and 0 current size. If all stays as
is, xrealloc_and_strcat (in subr.c) should change to not calculate and
send newsize because it isn't needed or used if x2realloc starts with
the original size (lenp).
Chris
Chris Bohn
newsize. I wasn't thinking; it does need it for the while loop that is
in the cvs 1.12.6 version of expand_string.
Chris
Derek Robert Price
Hash: SHA1
Chris Bohn wrote:
> Ignore my very last comments about xrealloc_and_strcat not needing
> newsize. I wasn't thinking; it does need it for the while loop that
> is in the cvs 1.12.6 version of expand_string.
I think I solved this. It had to do with how realloc &x2realloc liked
their NULL pointers. I ended up changing an initialization in client.c
to NULL and tweaking xstrcat_and_allocate to deal happily with a NULL
destination string.
Go ahead and check out the trunk if you'd like to try it now. The fix
will be in 1.12.7.
Derek
- --
*8^)
Email: de...@ximbiot.com
Get CVS support at <http://ximbiot.com>!
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)
Comment: Using GnuPG with Netscape - http://enigmail.mozdev.org
iD8DBQFAY5k9LD1OTBfyMaQRAreZAJ4oI9nLQlQf2EYh87hKMVIlLmCBMgCeMRvQ
uQvy7Cwt4Xgepsn3N9uJRNw=
=9g0L
-----END PGP SIGNATURE-----