=== 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
Taken by and assigned to: Keith McRae
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.
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.
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.
(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?
"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!
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
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 :-)
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; ... }
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
Keith, no reason for apologies. Do this with whatever pace suits you -- and let us know if we can help you with anything.
Hi Keith - wondering if you have a patch / way-point here :-)
And/or did this get finished ? :-) if so could we close this out ?
bug#56110 splits out the read/write methods instead of type-sensitive stream operators fun.
Keith: any update here?
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
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. > >
Noel kindly nailed this in: 16a2e903356520c90a9bf91c47265f79be12e74a Awesome work - thanks for that ! => closing.
Migrating Whiteboard tags to Keywords: (EasyHack,DifficultyBeginner,SkillCpp,TopicCleanup) [NinjaEdit]
Remove LibreOffice Dev List from CC on EasyHacks (curtailing excessive email to list) [NinjaEdit]