[Libreoffice-commits] update string copy semantics to be undefined in a non-crashing way

[Stephan Bergmann]

On 10/02/2012 07:40 PM, Libreoffice Gerrit user wrote:
> commit 595ba03fd4769c069bbe0431ab878956ef1c37ad
> Author: Michael Meeks <michael.meeks at suse.com>
> Date:   Tue Oct 2 18:39:30 2012 +0100
>
>      update string copy semantics to be undefined in a non-crashing way.
>
>      Change-Id: I03bb4db5931932280e368012cbaee6bef2854dd6
>
> diff --git a/sal/inc/rtl/string.hxx b/sal/inc/rtl/string.hxx
> index 84ab48e..d58999b 100644
> --- a/sal/inc/rtl/string.hxx
> +++ b/sal/inc/rtl/string.hxx
> @@ -1024,31 +1024,27 @@ public:
>       /**
>         Returns a new string that is a substring of this string.
>
> -      The substring begins at the specified beginIndex.  It is an error for
> -      beginIndex to be negative or to be greater than the length of this string.
> +      The substring begins at the specified beginIndex. If
> +      beginIndex is negative or be greater than the length of
> +      this string, behaviour is undefined.

Given that "it is an error for X to happen" and "if X happens, behaviour 
is undefined" have exactly the same meaning (at least in my 
understanding of computing), I wonder whether this is just a harmless 
rephrasing, or whether there is a deeper misunderstanding lurking there.

> diff --git a/sal/inc/rtl/ustring.hxx b/sal/inc/rtl/ustring.hxx
> index b008054..e12a4b0 100644
> --- a/sal/inc/rtl/ustring.hxx
> +++ b/sal/inc/rtl/ustring.hxx
> @@ -1372,17 +1368,9 @@ public:
>       */
>       OUString copy( sal_Int32 beginIndex, sal_Int32 count ) const SAL_THROW(())
>       {
> -        assert(beginIndex >= 0 && beginIndex <= getLength() && count >= 0
> -               && sal::static_int_cast< sal_uInt32 >(count) <=
> -                  sal::static_int_cast< sal_uInt32 >(getLength() - beginIndex));
> -        if ( (beginIndex == 0) && (count == getLength()) )
> -            return *this;
> -        else
> -        {
> -            rtl_uString* pNew = 0;
> -            rtl_uString_newFromStr_WithLength( &pNew, pData->buffer+beginIndex, count );
> -            return OUString( pNew, (DO_NOT_ACQUIRE*)0 );
> -        }
> +        rtl_uString *pNew = 0;
> +        rtl_uString_newFromSubString( &pNew, pData, beginIndex, count );
> +        return OUString( pNew, (DO_NOT_ACQUIRE*)0 );
>       }
>
>       /**
> diff --git a/sal/rtl/source/strtmpl.cxx b/sal/rtl/source/strtmpl.cxx
> index ebb0da1..6069349 100644
> --- a/sal/rtl/source/strtmpl.cxx
> +++ b/sal/rtl/source/strtmpl.cxx
> @@ -1194,6 +1194,29 @@ void SAL_CALL IMPL_RTL_STRINGNAME( newFromStr_WithLength )( IMPL_RTL_STRINGDATA*
>
>   /* ----------------------------------------------------------------------- */
>
> +void SAL_CALL IMPL_RTL_STRINGNAME( newFromSubString )( IMPL_RTL_STRINGDATA** ppThis,
> +                                                       const IMPL_RTL_STRINGDATA* pFrom,
> +                                                       sal_Int32 beginIndex,
> +                                                       sal_Int32 count )
> +    SAL_THROW_EXTERN_C()
> +{
> +    if ( beginIndex == 0 && count == pFrom->length )
> +    {
> +        IMPL_RTL_STRINGNAME( assign )( ppThis, const_cast< IMPL_RTL_STRINGDATA * >( pFrom ) );
> +        return;
> +    }
> +    if ( count < 0 || beginIndex < 0 || beginIndex + count > pFrom->length )

Note how the original code above prevented problems with overflowing 
beginIndex + count.

> +    {
> +        OSL_FAIL( "Out of bounds substring access" );

New code should use SAL_WARN, please.  (Arguably, it should rather use 
abort.  Which would also make the following code moot.)

> +        IMPL_RTL_STRINGNAME( newFromLiteral )( ppThis, "!!br0ken!!", 10, 0 );
> +        return;
> +    }
> +
> +    IMPL_RTL_STRINGNAME( newFromStr_WithLength )( ppThis, pFrom->buffer + beginIndex, count );
> +}
> +
> +/* ----------------------------------------------------------------------- */
> +
>   // Used when creating from string literals.
>   void SAL_CALL IMPL_RTL_STRINGNAME( newFromLiteral )( IMPL_RTL_STRINGDATA** ppThis,
>                                                        const sal_Char* pCharStr,
> diff --git a/sal/util/sal.map b/sal/util/sal.map
> index 7a431c3..9efed6a 100644
> --- a/sal/util/sal.map
> +++ b/sal/util/sal.map
> @@ -627,6 +627,12 @@ LIBO_UDK_3.6 { # symbols available in >= LibO 3.6
>   	rtl_uStringBuffer_makeStringAndClear;
>   } UDK_3.10;
>
> +LIBO_UDK_3.7 { # symbols available in >= LibO 3.7
> +    global:
> +        rtl_string_newFromSubString;
> +        rtl_uString_newFromSubString;
> +} UDK_3.10;

Oh, what should have been a linear list of versions turned into a 
multi-pronged fork starting with LIBO_UDK_3.5.  Fixed that now for 
LIBO_UDK_3.7 at least.

Stephan