You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2004/01/15 02:40:31 UTC
Re: AIX 5.1 client crash (buffer-overflow)
Travis <sv...@castle.fastmail.fm> writes:
[This is a subject for dev@s.t.o]
> I'm getting a crash from the following:
>
> subversion/libsvn_subr/subst.c:
> 561: readlen = sizeof (buf);
> 562: while (readlen == sizeof (buf))
> 563: {
> 564: SVN_ERR (svn_stream_read (s, buf, &readlen));
> 565: buf[readlen] = '\0';
>
> buf is 102401 bytes in size.
> readlen gets set to 102401.
> svn_stream_read does not modify readlen.
> buf[102401] is out-of-range and causes the crash.
>
> Clearly, something is wrong.
Most definitely! The line assigning to buf[102401] is overwriting one
byte of the stack. I suspect we have been getting away with this
because our usual platforms have padding bytes (at least 3) between
the end of buf and anything important, and they allow assignment to
those bytes.
Try the patch below. Probably a 1.0 candidate.
Index: subversion/libsvn_subr/subst.c
===================================================================
--- subversion/libsvn_subr/subst.c (revision 8293)
+++ subversion/libsvn_subr/subst.c (working copy)
@@ -558,8 +558,8 @@
assert (eol_str || keywords);
interesting = (eol_str && keywords) ? "$\r\n" : eol_str ? "\r\n" : "$";
- readlen = sizeof (buf);
- while (readlen == sizeof (buf))
+ readlen = sizeof (buf) - 1;
+ while (readlen == sizeof (buf) - 1)
{
SVN_ERR (svn_stream_read (s, buf, &readlen));
buf[readlen] = '\0';
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: AIX 5.1 client crash (buffer-overflow)
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2004-01-16 at 12:38, Philip Martin wrote:
> Hmm, that function does bit operations on char, and char could be
> signed. Perhaps it would be better to use unsigned char. Greg?
> - char secret[64];
> + unsigned char secret[64];
Whoops. Yes, that looks correct.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: AIX 5.1 client crash (buffer-overflow)
Posted by Philip Martin <ph...@codematters.co.uk>.
Travis <sv...@castle.fastmail.fm> writes:
> Yes, xlc, version 6 on AIX 5.1.
>
> I had to make these changes to successfully compile because they
> stopped compilation:
>
> "subversion/libsvn_ra_svn/cram.c", line 90.13: 1506-280 (E) Function
> argument
> assignment between types "unsigned char*" and "char*" is not allowed.
> "subversion/libsvn_ra_svn/cram.c", line 99.17: 1506-280 (E) Function
> argument
> assignment between types "unsigned char*" and "char*" is not allowed.
> "subversion/libsvn_ra_svn/cram.c", line 105.17: 1506-280 (E) Function
> argument
> assignment between types "unsigned char*" and "char*" is not allowed.
>
> % diff orig/subversion-0.35.1/subversion/libsvn_ra_svn/cram.c \
> subversion-0.35.1/subversion/libsvn_ra_svn/cram.c
> 90c90
> < apr_md5(secret, password, len);
> ---
> > apr_md5((unsigned char*)secret, password, len);
> 99c99
> < apr_md5_final(digest, &ctx);
> ---
> > apr_md5_final((unsigned char*)digest, &ctx);
> 105c105
> < apr_md5_final(digest, &ctx);
> ---
> > apr_md5_final((unsigned char*)digest, &ctx);
Hmm, that function does bit operations on char, and char could be
signed. Perhaps it would be better to use unsigned char. Greg?
Index: subversion/libsvn_ra_svn/cram.c
===================================================================
--- subversion/libsvn_ra_svn/cram.c (revision 8340)
+++ subversion/libsvn_ra_svn/cram.c (working copy)
@@ -78,7 +78,7 @@
static void compute_digest(char *digest, const char *challenge,
const char *password)
{
- char secret[64];
+ unsigned char secret[64];
apr_size_t len = strlen(password), i;
apr_md5_ctx_t ctx;
> "subversion/clients/cmdline/main.c", line 611.17: 1506-112 (E)
> Duplicate type
> qualifier "volatile" ignored.
>
> % diff orig/subversion-0.35.1/subversion/clients/cmdline/main.c \
> subversion-0.35.1/subversion/clients/cmdline/main.c
> 611c611
> < static volatile sig_atomic_t cancelled = FALSE;
> ---
> > static sig_atomic_t cancelled = FALSE;
Hmm, again. I think this is a real AIX bug. The C standard is
explicit, we need to use "volatile sig_atomic_t". I see that gcc's
fixincl removes volatile from the shadow sys/signal.h, but says that
only some versions of AIX are affected. We need to be very careful
not to remove volatile from our code in cases where there is no
duplicate. Perhaps we could use an autoconf macro. Or we could use
an integer type directly, but that's no more portable than not using
volatile. Or we could leave it as it is, but add a comment to the
code saying that if the error occurs then volatile should be removed.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: AIX 5.1 client crash (buffer-overflow)
Posted by Philip Martin <ph...@codematters.co.uk>.
Travis <sv...@castle.fastmail.fm> writes:
> Thanks Philip. That appeared to work and I now have what does appear
> to be a working svn client at first blush (wonderful!).
I believe you were using AIX's xlc compiler. It's always good to
check if an unusual compiler flags something different. Did it
produce any warnings while building Subversion?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: AIX 5.1 client crash (buffer-overflow)
Posted by Philip Martin <ph...@codematters.co.uk>.
Travis <sv...@castle.fastmail.fm> writes:
> Thanks Philip. That appeared to work and I now have what does appear
> to be a working svn client at first blush (wonderful!).
I believe you were using AIX's xlc compiler. It's always good to
check if an unusual compiler flags something different. Did it
produce any warnings while building Subversion?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@subversion.tigris.org
For additional commands, e-mail: users-help@subversion.tigris.org
Re: AIX 5.1 client crash (buffer-overflow)
Posted by Travis <sv...@castle.fastmail.fm>.
On Jan 14, 2004, at 8:40 PM, Philip Martin wrote:
> Travis <sv...@castle.fastmail.fm> writes:
>
> [This is a subject for dev@s.t.o]
Yeah, I'm a bit confused by the mailing lists. The mailing list
archives show the exact same content for users and dev, so I thought
that perhaps the lists were aliased at this time. But your comment
(and cross-posting to both lists) makes me think that maybe there's a
bug in the web archive browsing on s.t.o.
>> I'm getting a crash from the following:
>>
>> subversion/libsvn_subr/subst.c:
>> 561: readlen = sizeof (buf);
>> 562: while (readlen == sizeof (buf))
>> 563: {
>> 564: SVN_ERR (svn_stream_read (s, buf, &readlen));
>> 565: buf[readlen] = '\0';
>>
>> buf is 102401 bytes in size.
>> readlen gets set to 102401.
>> svn_stream_read does not modify readlen.
>> buf[102401] is out-of-range and causes the crash.
>>
>> Clearly, something is wrong.
>
> Most definitely! The line assigning to buf[102401] is overwriting one
> byte of the stack. I suspect we have been getting away with this
> because our usual platforms have padding bytes (at least 3) between
> the end of buf and anything important, and they allow assignment to
> those bytes.
>
> Try the patch below. Probably a 1.0 candidate.
>
>
> Index: subversion/libsvn_subr/subst.c
> ===================================================================
> --- subversion/libsvn_subr/subst.c (revision 8293)
> +++ subversion/libsvn_subr/subst.c (working copy)
> @@ -558,8 +558,8 @@
> assert (eol_str || keywords);
> interesting = (eol_str && keywords) ? "$\r\n" : eol_str ? "\r\n" :
> "$";
>
> - readlen = sizeof (buf);
> - while (readlen == sizeof (buf))
> + readlen = sizeof (buf) - 1;
> + while (readlen == sizeof (buf) - 1)
> {
> SVN_ERR (svn_stream_read (s, buf, &readlen));
> buf[readlen] = '\0';
>
> --
> Philip Martin
Thanks Philip. That appeared to work and I now have what does appear
to be a working svn client at first blush (wonderful!).
-Travis Pouarz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: AIX 5.1 client crash (buffer-overflow)
Posted by Travis <sv...@castle.fastmail.fm>.
On Jan 14, 2004, at 8:40 PM, Philip Martin wrote:
> Travis <sv...@castle.fastmail.fm> writes:
>
> [This is a subject for dev@s.t.o]
Yeah, I'm a bit confused by the mailing lists. The mailing list
archives show the exact same content for users and dev, so I thought
that perhaps the lists were aliased at this time. But your comment
(and cross-posting to both lists) makes me think that maybe there's a
bug in the web archive browsing on s.t.o.
>> I'm getting a crash from the following:
>>
>> subversion/libsvn_subr/subst.c:
>> 561: readlen = sizeof (buf);
>> 562: while (readlen == sizeof (buf))
>> 563: {
>> 564: SVN_ERR (svn_stream_read (s, buf, &readlen));
>> 565: buf[readlen] = '\0';
>>
>> buf is 102401 bytes in size.
>> readlen gets set to 102401.
>> svn_stream_read does not modify readlen.
>> buf[102401] is out-of-range and causes the crash.
>>
>> Clearly, something is wrong.
>
> Most definitely! The line assigning to buf[102401] is overwriting one
> byte of the stack. I suspect we have been getting away with this
> because our usual platforms have padding bytes (at least 3) between
> the end of buf and anything important, and they allow assignment to
> those bytes.
>
> Try the patch below. Probably a 1.0 candidate.
>
>
> Index: subversion/libsvn_subr/subst.c
> ===================================================================
> --- subversion/libsvn_subr/subst.c (revision 8293)
> +++ subversion/libsvn_subr/subst.c (working copy)
> @@ -558,8 +558,8 @@
> assert (eol_str || keywords);
> interesting = (eol_str && keywords) ? "$\r\n" : eol_str ? "\r\n" :
> "$";
>
> - readlen = sizeof (buf);
> - while (readlen == sizeof (buf))
> + readlen = sizeof (buf) - 1;
> + while (readlen == sizeof (buf) - 1)
> {
> SVN_ERR (svn_stream_read (s, buf, &readlen));
> buf[readlen] = '\0';
>
> --
> Philip Martin
Thanks Philip. That appeared to work and I now have what does appear
to be a working svn client at first blush (wonderful!).
-Travis Pouarz
---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@subversion.tigris.org
For additional commands, e-mail: users-help@subversion.tigris.org