Document Foundation Mail Archive › LibreOffice › Dev

[PATCH] Fixed out of bounds memory access

Classic

List

Threaded

6 messages Options

Marc-André Laverdière-2

[PATCH] Fixed out of bounds memory access

Here is a patch for a small fish I caught while valgrinding. It was
accessing memory in the strdup.

Marc-André LAVERDIÈRE
"Perseverance must finish its work so that you may be mature and complete,
not lacking anything." -James 1:4
http://asimplediscipleslife.blogspot.com/
mlaverd.theunixplace.com

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

0001-Fixed-invalid-memory-access.patch (2K) Download Attachment

Stephan Bergmann-2

Re: [PATCH] Fixed out of bounds memory access

On 06/15/2012 05:45 PM, Marc-André Laverdière wrote:
> Here is a patch for a small fish I caught while valgrinding. It was
> accessing memory in the strdup.

Was it really? My reading of the original

> rtl::OUString aUserName;
> rtl::OString aUser;
> oslSecurity aSec = osl_getCurrentSecurity();
> if( aSec )
> {
> osl_getUserName( aSec, &aUserName.pData );
> aUser = rtl::OUStringToOString( aUserName, osl_getThreadTextEncoding() );
> osl_freeSecurityHandle( aSec );
> }
>
> pSmProps[ 3 ].name = const_cast<char*>(SmUserID);
> pSmProps[ 3 ].type = const_cast<char*>(SmARRAY8);
> pSmProps[ 3 ].num_vals = 1;
> pSmProps[ 3 ].vals = new SmPropValue;
> pSmProps[ 3 ].vals->value = strdup( aUser.getStr() );
> pSmProps[ 3 ].vals->length = strlen( (char *)pSmProps[ 3 ].vals->value )+1;

is that at the end aUser is either the empty string (if !aSec) or holds
on to an OString copy of the data obtained from osl_getUserName. In
either case, the strdup(aUser.getStr()) should be OK?

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Caolán McNamara

Re: [PATCH] Fixed out of bounds memory access

On Mon, 2012-06-18 at 13:41 +0200, Stephan Bergmann wrote:
> In either case, the strdup(aUser.getStr()) should be OK?

I bet this is one if the false-positive occasions where valgrind isn't
aware of one of the strlen performance hacks IIRC where glibc knows that
it can get away with traversing that strdup's memory block in 4byte
chunks in its strlen e.g. someone with a 4/8 character length username
wouldn't see it :-)

So... if you just changed the strlen to be aUser.getLength() + 1 would
that silence valgrind too ?

C.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Stephan Bergmann-2

Re: [PATCH] Fixed out of bounds memory access

On 06/18/2012 05:50 PM, Caolán McNamara wrote:
> I bet this is one if the false-positive occasions where valgrind isn't
> aware of one of the strlen performance hacks IIRC where glibc knows that
> it can get away with traversing that strdup's memory block in 4byte
> chunks in its strlen e.g. someone with a 4/8 character length username
> wouldn't see it :-)
>
> So... if you just changed the strlen to be aUser.getLength() + 1 would
> that silence valgrind too ?

As it happened, I ran into this false valgrind warning now, too, and
addressed it with
<http://cgit.freedesktop.org/libreoffice/core/commit/?id=97beabccb73321a8d2e022705afa755f15e99fa0>
"http://cgit.freedesktop.org/libreoffice/core/commit/?id=97beabccb73321a8d2e022705afa755f15e99fa0."
(Argh, now that I re-read your above suggestion, using
rtl::OString::getLength would really have been nicer than
rtl_str_getLength. Anyway...)

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Caolán McNamara

Re: [PATCH] Fixed out of bounds memory access

On Wed, 2012-06-20 at 14:48 +0200, Stephan Bergmann wrote:
> rtl_str_getLength

I wonder if there would be any actual measurable performance benefit to
sticking in some hackery to keep the rtl_str_getLength symbol for
backwards compatibility but alias/define rtl_str_getLength to strlen in
order to get any benefit of those optimized strlens. Or do they only
kick in if the compiler can determine locally that the argument comes
from malloc ? dunno.

C.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Stephan Bergmann-2

Re: [PATCH] Fixed out of bounds memory access

On 06/20/2012 10:22 PM, Caolán McNamara wrote:
> I wonder if there would be any actual measurable performance benefit to
> sticking in some hackery to keep the rtl_str_getLength symbol for
> backwards compatibility but alias/define rtl_str_getLength to strlen in
> order to get any benefit of those optimized strlens.

My gut assumption is that it wouldn't make too much of a difference in
practice. (Do we have large amounts of calls to rtl_str_getLength to
begin with?)

And it would shoot down the valgrind workaround. ;)

> Or do they only
> kick in if the compiler can determine locally that the argument comes
> from malloc ? dunno.

The actual optimization is likely (only) to read in complete words
rather than individual bytes, which can be applied independently of the
provenance of the argument. (In fact, a compiler could even
automatically optimize the implementation of rtl_str_getLength in that way.)

Stephan
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice