Bug 39428 - audit / remove SvStream long operators
Summary: audit / remove SvStream long operators
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
3.5.0 Beta3
Hardware: Other All
: medium normal
Assignee: Keith McRae
URL:
Whiteboard:
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2011-07-21 07:34 UTC by Björn Michaelsen
Modified: 2016-02-18 16:37 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Michaelsen 2011-07-21 07:34:16 UTC
=== audit / remove SvStream long operators ===

'''Background:''' the SvStream class has many operators used for creating binary streams, some of these are not particularly helpful - using 'long' for example, is confusing, as long can have a different size on different platforms, and the stream will serialize only the 4 bytes of it. We should remove these SvStream methods, find and replace callers with an explicit sal_uInt32 cast, call.

'''Skills:''' building, simple C++ programming
Comment 1 Keith McRae 2011-12-19 03:17:25 UTC
Taken by and assigned to: Keith McRae
Comment 2 Keith McRae 2012-01-04 02:56:15 UTC
Current auditing usage of SvStream operator(s) <</>> throughout the source code. I hope to have completed this by Friday 4th Jan 2011. Will mail results to mailing list.
Comment 3 Keith McRae 2012-01-16 08:21:15 UTC
Audit of usage complete. SvStream class modified, other classes using operator>>/<<(long) modified. Compilation and link of build successful.
Currently investigating unit test failure. Target completion date Weds 18th Jan.
Comment 4 Stephan Bergmann 2012-01-19 03:00:25 UTC
Thinking about it, for a class like SvStream that stores binary representations of data (i.e., four bytes to represent a 32 bit integer, as opposed to classes like std::stream that use variable-length textual representations), overloading operator << and >>, at least for the various numerical types, is a bad idea.  The best long-term fix would probably be to replace those overloaded operators with dedicated functions like readInt16, readInt32, writeInt16, writeInt32, etc.
Comment 5 Keith McRae 2012-01-19 03:53:37 UTC
(In reply to comment #4)
> Thinking about it, for a class like SvStream that stores binary representations
> of data (i.e., four bytes to represent a 32 bit integer, as opposed to classes
> like std::stream that use variable-length textual representations), overloading
> operator << and >>, at least for the various numerical types, is a bad idea. 
> The best long-term fix would probably be to replace those overloaded operators
> with dedicated functions like readInt16, readInt32, writeInt16, writeInt32,
> etc.

I agree wholeheartedly! As I've already got experience in this area, I'll take this on. Should all the operator << and >> instances be replaced? I'm thinking, for clarity, yes?
Comment 6 Stephan Bergmann 2012-01-19 04:13:10 UTC
"Should all the operator << and >> instances be replaced? I'm thinking,
for clarity, yes?"  Would say so, too.  Thanks for picking up this herculenean task!
Comment 7 Caolán McNamara 2012-01-23 02:44:55 UTC
I live with a constant sense of dread that e.g. someone changes a xub_StrLen of 16bits to an sal_Int32 of 32bits when changing a String to an OUString and streaming the sal_Int32 out silently via << and busting some binary file format, explicit type write/reads appeals to me far more as a safer option
Comment 8 Michael Meeks 2012-02-01 07:03:00 UTC
I guess - if we are doing this very valuable cleanup; that while we are doing it - we should ensure that the 'ReadInt()' methods (or whatever) return a defined default result - ie. zero for ints, false for bools, "" for strings etc. ie.

sal_Int32 readInt32(sal_Int32 nDefault = 0);

We should take care as we do the change to catch those (rather few) places where the undefined result is used as a feature: ie. "if we failed to read, the default was the old value of the result". ie. this sort of thing:

SvStream& SvStream::operator>>(sal_uInt16& r)
{
    sal_uInt16 n = 0;
    READNUMBER_WITHOUT_SWAP(sal_uInt16, n)
    if (good())
    {
        if (bSwap)
            SwapUShort(n);
        r = n;
    }
    return *this;
}

is really bad news for security auditing - and it'd be lovely to kill it at the same time as this :-)
Comment 9 Caolán McNamara 2012-02-01 07:09:32 UTC
here's one place I know about where the current std::stream-alike "failed reads leave the original value untouched" is relied upon

sw/source/filter/ww8/ww8san.cxx
    while( 1 )
    {
        sal_uInt8 clxt(2);
        *pStr >> clxt; //if read fails, then value remains 2, so loop breaks even on failure
        nLeft--;
        if( 2 == clxt )
            break;
        ... 
    }
Comment 10 Keith McRae 2012-02-21 04:25:22 UTC
Apologies for lack of communication, holidays & paid work interrupted my flow :)

Progress report:

tools/ has been updated with the new methods and compiles cleanly. However, I'm currently investigating a failure within the unit tests. Should have this nailed within a day or two.

Keith
Comment 11 Stephan Bergmann 2012-02-21 05:20:26 UTC
Keith, no reason for apologies.  Do this with whatever pace suits you -- and let us know if we can help you with anything.
Comment 12 Michael Meeks 2012-10-18 01:03:21 UTC
Hi Keith - wondering if you have a patch / way-point here :-)
Comment 13 Michael Meeks 2012-10-18 01:04:05 UTC
And/or did this get finished ? :-) if so could we close this out ?
Comment 14 Michael Meeks 2012-10-18 01:17:44 UTC
bug#56110 splits out the read/write methods instead of type-sensitive stream operators fun.
Comment 15 Julien Nabet 2013-09-14 09:29:30 UTC
Keith: any update here?
Comment 16 Björn Michaelsen 2013-10-04 18:46:21 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 17 Keith McRae 2013-10-22 11:05:46 UTC
Hi Julien

Sorry for the delay in replying.
I'm currently involved in some projects that are eating up all of my time.
I'm not sure when I'll be able to get back to it to be honest.

Kind regards
Keith


On Sat, Sep 14, 2013 at 10:29 AM, <bugzilla-daemon@freedesktop.org> wrote:

>  Julien Nabet <serval2412@yahoo.fr> changed bug 39428<https://bugs.freedesktop.org/show_bug.cgi?id=39428>
>  What Removed Added  CC   serval2412@yahoo.fr
>
>  *Comment # 15 <https://bugs.freedesktop.org/show_bug.cgi?id=39428#c15>on bug
> 39428 <https://bugs.freedesktop.org/show_bug.cgi?id=39428> from Julien
> Nabet <serval2412@yahoo.fr> *
>
> Keith: any update here?
>
>  ------------------------------
> You are receiving this mail because:
>
>    - You are on the CC list for the bug.
>    - You are the assignee for the bug.
>
>
Comment 18 Michael Meeks 2014-07-26 21:59:08 UTC
Noel kindly nailed this in: 16a2e903356520c90a9bf91c47265f79be12e74a

Awesome work - thanks for that ! => closing.
Comment 19 Robinson Tryon (qubit) 2015-12-15 16:27:28 UTC
Migrating Whiteboard tags to Keywords: (EasyHack,DifficultyBeginner,SkillCpp,TopicCleanup)
[NinjaEdit]
Comment 20 Robinson Tryon (qubit) 2016-02-18 16:37:07 UTC
Remove LibreOffice Dev List from CC on EasyHacks
(curtailing excessive email to list)
[NinjaEdit]