You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C.A.T.Magic" <c....@gmx.at> on 2004/05/01 16:05:31 UTC
some 'itoa'+apr_palloc issues and a few compiler warnings
Hi subversion devlopers,
I noticed the use of "atoi" in some places followed
by an apr-malloc on the returnvalue without doing any
checks on the returnvalue first.
I'd prefer the use of a string-to-int function with
better error and range checking
(since cases like out-of-range numbers "374623874682734"
invalid characters "+23", "16.2", "err" just
return 0 or other unexpected values with atoi)
======
for example in the new fsfs.c :
>>
size_t keylen = (size_t) atoi (stringbuf->data + 2);
/* Now read that much into a buffer, + 1 byte for null terminator */
void *keybuf = apr_palloc (pool, keylen + 1);
SVN_ERR (svn_stream_read (stream, keybuf, &keylen));
((char *) keybuf)[keylen] = '\0';
<<
for example if "-1" would be returned from atoi then the '\0'
could go into dead memory.
how do apr_palloc(0) and apr_palloc(-2) react?
some similar atoi based alloc code exists in 'load.c' and 'hash.c'(2x)
and maybe other places too.
======
also 'parse_url' from client.c is using atoi on the portnumber
and leads to strange errors if a typo is using characters
where the port number is expected, like in
svn checkout http://some.url.at:X/repos/file
svn checkout http://some.url.at:0/repos/file
svn checkout http://some.url.at:80XYZ/repos/file
svn checkout http://some.url.at:/repos/file
which creates a buggy working copy.
=======
And maybe you find this compiler output (below) helpful too.
- but its nothing serious.
MSVC 7.1 (X86) Build of trunk 9593
=========================
* ...\subversion\libsvn_repos\reporter.c(136) : warning C4244:
'function' : conversion from 'apr_uint64_t' to 'apr_size_t', possible
loss of data
* ...\subversion\libsvn_repos\reporter.c(137) : warning C4244:
'function' : conversion from 'apr_uint64_t' to 'apr_size_t', possible
loss of data
* ...\subversion\libsvn_repos\reporter.c(153) : warning C4244: '=' :
conversion from 'apr_uint64_t' to 'svn_revnum_t', possible loss of data
* ...\subversion\libsvn_ra_svn\marshal.c(509) : warning C4244:
'function' : conversion from 'apr_uint64_t' to 'apr_size_t', possible
loss of data
* ...\subversion\libsvn_ra_svn\marshal.c(510) : warning C4244:
'function' : conversion from 'apr_uint64_t' to 'apr_size_t', possible
loss of data
* ...\subversion\libsvn_ra_svn\marshal.c(515) : warning C4244: '=' :
conversion from 'apr_uint64_t' to 'apr_size_t', possible loss of data
* ...\subversion\libsvn_ra_svn\marshal.c(599) : warning C4244: '=' :
conversion from 'apr_uint64_t' to 'svn_revnum_t', possible loss of data
* ...\subversion\libsvn_ra_svn\marshal.c(738) : warning C4244:
'function' : conversion from 'apr_uint64_t' to 'apr_status_t', possible
loss of data
* ...\subversion\libsvn_ra_svn\marshal.c(740) : warning C4244: '=' :
conversion from 'apr_uint64_t' to 'long', possible loss of data
* ...\subversion\libsvn_ra_svn\client.c(454) : warning C4090:
'function' : different 'const' qualifiers
* ...\subversion\libsvn_ra_svn\client.c(569) : warning C4244: '=' :
conversion from 'apr_uint64_t' to 'int', possible loss of data
* ...\subversion\libsvn_client\blame.c(237) : warning C4101:
'change' : unreferenced local variable
* ...\subversion\libsvn_delta\text_delta.c(335) : warning C4244:
'function' : conversion from 'svn_filesize_t' to 'apr_size_t', possible
loss of data
* ...\subversion\libsvn_diff\diff_file.c(149) : warning C4244:
'function' : conversion from 'apr_off_t' to 'apr_size_t', possible loss
of data
* ...\subversion\libsvn_diff\diff_file.c(163) : warning C4244:
'function' : conversion from 'apr_off_t' to 'apr_size_t', possible loss
of data
* ...\subversion\libsvn_diff\diff_file.c(165) : warning C4244:
'function' : conversion from 'apr_off_t' to 'apr_size_t', possible loss
of data
* ...\subversion\libsvn_diff\diff_file.c(202) : warning C4244: '=' :
conversion from 'apr_off_t' to 'apr_size_t', possible loss of data
* ...\subversion\libsvn_diff\diff_file.c(256) : warning C4244: '=' :
conversion from 'apr_off_t' to 'int', possible loss of data
* ...\subversion\libsvn_diff\diff_file.c(305) : warning C4244: '=' :
conversion from 'apr_off_t' to 'apr_size_t', possible loss of data
* ...\subversion\libsvn_diff\diff_file.c(397) : warning C4244:
'function' : conversion from 'apr_off_t' to 'apr_size_t', possible loss
of data
* ...\subversion\libsvn_diff\diff_file.c(408) : warning C4244:
'function' : conversion from 'apr_off_t' to 'size_t', possible loss of data
* ...\subversion\libsvn_fs_fs\fs_fs.c(592) : warning C4244: '=' :
conversion from 'apr_int64_t' to 'apr_size_t', possible loss of data
* ...\subversion\libsvn_fs_fs\fs_fs.c(599) : warning C4244: '=' :
conversion from 'apr_int64_t' to 'apr_size_t', possible loss of data
* ...\subversion\libsvn_fs_fs\fs_fs.c(997) : warning C4244: '=' :
conversion from 'apr_int64_t' to 'apr_size_t', possible loss of data
* ...\subversion\libsvn_fs_fs\fs_fs.c(1441) : warning C4244: '=' :
conversion from 'apr_off_t' to 'apr_size_t', possible loss of data
* ...\subversion\libsvn_fs_fs\fs_fs.c(2949) : warning C4244: '=' :
conversion from 'apr_off_t' to 'apr_size_t', possible loss of data
* ...\subversion\libsvn_fs_fs\fs_fs.c(2952) : warning C4244: '=' :
conversion from 'svn_filesize_t' to 'apr_size_t', possible loss of data
* ...\subversion\svndumpfilter\main.c(54) : warning C4042: 'open_fn'
: has bad storage class
* ...\subversion\svnserve\serve.c(1085) : warning C4244: '=' :
conversion from 'apr_uint64_t' to 'int', possible loss of data
* ...\subversion\svnadmin\main.c(64) : warning C4042: 'open_fn' :
has bad storage class
=======
:-)
c.a.t.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: some 'itoa'+apr_palloc issues and a few compiler warnings
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>The case I am most concerned about is svndiff.c, because, unlike hash
>dumps and FSFS files, we accept svndiffs over the network. It seems
>like one way to DOS a server would be to send an svndiff window with a
>huge instruction or new data length, and then get the server to buffer
>some large amount of data. You'd need commit access to do that, so it's
>not so bad, but I have thought about proposing a "maximum reasonable
>window size" constant to address the issue.
>
>
We have such a constant, it's callsd SVN_MAX_OBJECT_SIZE. All we have to
do is agree on its value; right now it't the maximum allocatable size,
which is not very useful.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: some 'itoa'+apr_palloc issues and a few compiler warnings
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sat, 2004-05-01 at 12:05, C.A.T.Magic wrote:
> for example in the new fsfs.c :
> >>
> size_t keylen = (size_t) atoi (stringbuf->data + 2);
[...]
> for example if "-1" would be returned from atoi then the '\0'
> could go into dead memory.
> how do apr_palloc(0) and apr_palloc(-2) react?
size_t is an unsigned type. (We should be using apr_size_t there, but
that's an orthogonal issue.) So while reading -1 would cause us to
attempt to allocate a ridiculous amount of memory, it would not cause a
buffer overrun.
> some similar atoi based alloc code exists in 'load.c' and 'hash.c'(2x)
> and maybe other places too.
The case I am most concerned about is svndiff.c, because, unlike hash
dumps and FSFS files, we accept svndiffs over the network. It seems
like one way to DOS a server would be to send an svndiff window with a
huge instruction or new data length, and then get the server to buffer
some large amount of data. You'd need commit access to do that, so it's
not so bad, but I have thought about proposing a "maximum reasonable
window size" constant to address the issue.
> also 'parse_url' from client.c is using atoi on the portnumber
> and leads to strange errors if a typo is using characters
> where the port number is expected, like in
> svn checkout http://some.url.at:X/repos/file
> svn checkout http://some.url.at:0/repos/file
> svn checkout http://some.url.at:80XYZ/repos/file
> svn checkout http://some.url.at:/repos/file
client.c applies to the svn:// protocol, not the http:// protocol. I
don't get particularly strange behavior from those examples or from
variants of them.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org