a bug in svn_fs_base__unparse_representation_skel?

[Mary Jane Lemur <mj...@gmail.com>]

i'm looking in libsvn_fs_base/util/fs_skels.c, line 1038.

the code seems to be duplicating the rep->checksum array in memory by
calling svn_fs_base__mem_atom.  i understand that it's passing
rep->checksum as the 'data' argument, but for the 'length' argument
its passing

APR_MD5_DIGESTSIZE / sizeof (*(rep->checksum)

isnt this wrong?   if we're on a system where (unsigned char) is 2
bytes, then dont we want to allocate a memory space thats twice as
big?  instead of dividing, shouldnt it be multiplying by the sizeof()?
 like

APR_MD5_DIGESTSIZE * sizeof (*(rep->checksum)

i'm guessing that this bug hasnt been noticed because every system so
far has a 1-byte unsigned char, so we're dividing APR_MD5_DIGESTSIZE
by 1, which works out ok.  just lucky, mebbe?

--mj

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: a bug in svn_fs_base__unparse_representation_skel?

[Julian Foad <ju...@btopenworld.com>]

Mary Jane Lemur wrote:
> i'm looking in libsvn_fs_base/util/fs_skels.c, line 1038.
> 
> the code seems to be duplicating the rep->checksum array in memory by
> calling svn_fs_base__mem_atom.  i understand that it's passing
> rep->checksum as the 'data' argument, but for the 'length' argument
> its passing
> 
> APR_MD5_DIGESTSIZE / sizeof (*(rep->checksum)
> 
> isnt this wrong?   if we're on a system where (unsigned char) is 2
> bytes, then dont we want to allocate a memory space thats twice as
> big?  instead of dividing, shouldnt it be multiplying by the sizeof()?
>  like
> 
> APR_MD5_DIGESTSIZE * sizeof (*(rep->checksum)

Yes; or, at least, that's better than dividing by it.

> i'm guessing that this bug hasnt been noticed because every system so
> far has a 1-byte unsigned char, so we're dividing APR_MD5_DIGESTSIZE
> by 1, which works out ok.  just lucky, mebbe?

Most of the code assumes a char is one byte.  svn_fs_base__mem_atom() and 
sizeof() both work in bytes.  APR doesn't specify (in apr_md5.h) what the units 
of APR_MD5_DIGESTSIZE are, but we treat it as if the units are bytes or chars 
interchangeably.  Therefore the fix is:

   APR_MD5_DIGESTSIZE

(not multiplied or divided by anything), just like everywhere else it is used 
for "memcpy()" etc.  An alternative would be:

   sizeof(rep->checksum)

but that wouldn't work if rep->checksum were to be changed to a pointer rather 
than a fixed array.

Fixed in r17704 (along with some useless backslashes that I happened to notice).

Thank you for pointing it out.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: a bug in svn_fs_base__unparse_representation_skel?

[Philip Martin <ph...@codematters.co.uk>]

Mary Jane Lemur <mj...@gmail.com> writes:

> i'm looking in libsvn_fs_base/util/fs_skels.c, line 1038.
>
> the code seems to be duplicating the rep->checksum array in memory by
> calling svn_fs_base__mem_atom.  i understand that it's passing
> rep->checksum as the 'data' argument, but for the 'length' argument
> its passing
>
> APR_MD5_DIGESTSIZE / sizeof (*(rep->checksum)
>
> isnt this wrong?   if we're on a system where (unsigned char) is 2
> bytes,

The ISO C standard defines sizeof char to be 1.  As far as I know
systems that are "unusual", things like 32 bit DSP's, usually use
something like sizeof char == sizeof int == 1.  Do you know of any
systems where sizeof char is not 1?  I'd expect loads of code to fail
on any such non-standard system.

> then dont we want to allocate a memory space thats twice as big?
> instead of dividing, shouldnt it be multiplying by the sizeof()?
> like
>
> APR_MD5_DIGESTSIZE * sizeof (*(rep->checksum)
>
> i'm guessing that this bug hasnt been noticed because every system so
> far has a 1-byte unsigned char, so we're dividing APR_MD5_DIGESTSIZE
> by 1, which works out ok.  just lucky, mebbe?

That said the code does look odd; I think it should be either
sizeof(rep->checksum) or APR_MD5_DIGESTSIZE without any multiplication
or division.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org