[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Suspicious code in koffice-1.5.0-rc1
From:       David Faure <faure () kde ! org>
Date:       2006-04-10 23:30:58
Message-ID: 200604110130.59212.faure () kde ! org
[Download RAW message or body]

On Sunday 09 April 2006 16:22, Christoph Bartoschek wrote:
> - lib/kofficecore/KoDocument.cpp:1458
> This case is not possible becuase it is excluded in line 1451.
Right; this is just to silence compiler warnings about unhandled switch cases though.

> - kword/KWTableFrameSet.cpp:805
> If cols stays 0 because the loop in line 803 is not entered, then you
> have a division by 0.
True, but a table without columns can't happen - hopefully...

> - kword/KWView.cpp:211
> The return value is "bool". Why not int?
Typo. Fixed.

> - kpresenter/KPrPage.cpp:911
> 
> The if statement has an empty then-clause. There is a ;
Fixed.

> - kpresenter/KPrDocument.cpp:3239
> 
> data is allocated by operator new[] and should be deleted by delete [].
Fixed.

> why not using std::vector<>
or QByteArray, indeed.

> - kword/KWStatisticsDialog.cpp:171
> "&& false" ?
Hmm. This disables the count of selected paragraphs instead of just "all paragraphs"
paragraphs += frameSet->paragraphsSelected();
Thomas?

> - lib/kofficecore/KoApplication.cpp:199
> If execution reaches line 187, then line 199 will crash.
Can't happen.

> - lib/kotext/KoRichText.cpp:202
> - lib/kotext/KoTextParag.cpp:242
> - lib/kotext/KoTextFormatter.cpp:353
Fixed

> - lib/kotext/KoTextObject.cpp:2015
> If p is NULL as indicated by line 2010, then line 2015 crashes.
Oh.. Fixed.

> - lib/kotext/KoTextObject.cpp:1324
> - lib/kotext/KoTextObject.cpp:1391 (similar)
Ah, just a useless null-check there.

> - lib/kotext/KoRichText.cpp:181 (similar)
> - lib/kotext/KoRichText.cpp:153 (similar)
In KoTextParag, doc is never 0, even if the code where this was taken from, had support for doc==0.

> - lib/kotext/KoTextObject.cpp:845
> If macroCmd is NULL here because in line 838 the else part is chosen,
> then this line crashes.
But the same test as in line 838 is used inside setParagLayoutCommand, to know whether
to return something or not. So if createUndoRedo is false, macroCmd is null but cmd is null too.
The tool is showing its limits there ;-)

> - lib/kotext/KoTextView.cpp:883
> If f can be NULL here, than line 877 crashes.
Right; it can't be. Check removed.

> - lib/kofficecore/KoFilterChain.cpp:922
> The for loop in line 918 is only entered if v is NULL, but then line 922 crashes.
Good catch!

> - lib/kofficecore/KoMainWindow.cpp:594
> If newdoc is NULL, then the condition in line 592 is true and line 594 crashes.
Brr, indeed, this code makes no sense. newdoc is already used many times before those lines anyway.
I added a check right after createDoc().

> - lib/kofficecore/KoDocument.cpp:930
> If doc is NULL as indicated by line 921, then line 930 crashes.
Fixed.

> - kword/KWCanvas.cpp:459
> If frame is NULL, then fs also becomes NULL and line 458 crashes.
Wow. Fixed.

> - kword/KWPartFrameSet.cpp:102
> If m_child is NULL as indicated by line 100, then line 102 crashes.
Fixed.

> - koshell/koshell_shell.cc:211
> If newdoc is NULL as indicated by line 209, then line 211 crashes.
Fixed.

> - kword/KWTextFrameSet.cpp:2131
> If page is NULL as line 2045 indicates, then line 2131 crashes.
Thomas?

> - kword/KWTextFrameSet.cpp:1079
> If pageWidth is NULL as line 971 indicates, then line 1079 crashes.
Ah, but 
 if ( marginLeft ) { assert( marginRight ); assert( pageWidth ); }
that's a contract with the caller. So pageWidth can't be NULL when marginLeft==true.
OK, rather tricky for a static analysis tool :)

[...]
Oh boy, so many other hits in kword... OK I keep some for tomorrow ;)

> - lib/kotext/KoParagLayout.cpp:391 !!!
> - lib/kotext/KoParagLayout.cpp:380 !!!
Wahhahhahh I suck.. bug wasn't noticed because this is for 1.2 compat.

> - lib/kotext/KoTextParag.cpp:1918
No-op, fortunately, but still better with break of course.

> - kword/KWDocument.cpp:747
Ditto (by chance...)

> - kword/KWDocument.cpp:4608
Error case anyway

> - lib/kotext/KoComplexText.cpp:1214
> - lib/kotext/KoComplexText.cpp:1162
Eek. No idea about that code, it's from Qt. I better not touch it.

Phew.

Christoph: many thanks for this data.

-- 
David Faure, faure@kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).

_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]