Re: Crash with gnome#627420-1.ods in string related function

On Tuesday 08 of January 2013, Michael Meeks wrote:
> On Tue, 2013-01-08 at 05:19 -0800, julien2412 wrote:
> > Hello Michael,
> >
> > Reading this thread, I took a look at sal/rtl/source/strtmpl.cxx
> >     969 static IMPL_RTL_STRINGDATA* IMPL_RTL_STRINGNAME( ImplAlloc )(
> > sal_Int32 nLen )
>
> ...
>
> > Since we cast "nLen" parameter in "sal_uInt32", could it help to add an
> > assert about nLen should be >= 0 ?

 Could help, hopefully shouldn't hurt.

>       Sounds great to me :-) lots of code paths to there check for a positive
> length I guess some mass code clean to use sal_uInt32 instead of
> sal_Int32 for string lengths might be in order at some stage - though
> perhaps there is some reason that's not obvious to me for strings to
> have a signed length and to check for it to be > 0 around the place:
> perhaps for some insertion/offset API semantic corner-case reason ?

 It is a fallacy that using an unsigned variable to represent values that 
cannot be negative actually buys you safety.

 E.g. make a function that takes unsigned int, pass -1 to it and see how the 
silly C/C++ int->unsigned int promotion lets you get away with it. Even 
without a warning with GCC, apparently -Wconversion doesn't catch this in 
4.7, Clang warns with -Wsign-conversion, but that triggers sooo many 
warnings, because signed types are simply inevitable. There is one thing that 
you will get though, and that's comparison warnings about mixing signed and 
unsigned types, and all the useless casts in our code that hide them.

 It's not like OUString needs to hold a string that's more than 2G characters 
long anyway.

-- 
 Lubos Lunak
 l.lunak@suse.cz