Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx

classic Classic list List threaded Threaded
6 messages Options
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx

Hello,

1)
Trying to reproduce fdo#47466 with master sources updated this morning, I had this:
#0  0x00007ff8cd039475 in *__GI_raise (sig=<optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00007ff8cd03c6f0 in *__GI_abort () at abort.c:92
#2  0x00007ff8cd8de31d in __gnu_debug::_Error_formatter::_M_error() const () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007ff8ae7c3f24 in __gnu_debug::_Safe_iterator<std::_Rb_tree_const_iterator<short>, std::__debug::set<short, std::less<short>, std::allocator<short> > >::operator* (
    this=0x7fff96484bf0) at /usr/include/c++/4.7/debug/safe_iterator.h:261
#4  0x00007ff8aee0bd21 in ScDocFunc::AutoFormat (this=0x288bd00, rRange=..., pTabMark=0x297b6d0, nFormatNo=1, bRecord=true, bApi=false)
    at /home/julien/compile-libreoffice/libo/sc/source/ui/docshell/docfunc.cxx:3743
#5  0x00007ff8af222bd0 in ScViewFunc::AutoFormat (this=0x296a7c0, nFormatNo=1, bRecord=1 '\001') at /home/julien/compile-libreoffice/libo/sc/source/ui/view/viewfun2.cxx:1553
#6  0x00007ff8af102636 in ScCellShell::Execute (this=0x29a45a0, rReq=...) at /home/julien/compile-libreoffice/libo/sc/source/ui/view/cellsh3.cxx:843
...

on console:
Objects involved in the operation:
iterator "this" @ 0x0x7fff96484bf0 {
type = N11__gnu_debug14_Safe_iteratorISt23_Rb_tree_const_iteratorIsENSt7__debug3setIsSt4lessIsESaIsEEEEE (mutable iterator);
  state = past-the-end;
  references sequence with type `NSt7__debug3setIsSt4lessIsESaIsEEE' @ 0x0x7fff96484bf0
}

I noticed this:
   3740             ScMarkData::iterator itr = aMark.begin(), itrEnd = aMark.end();
   3741             for (; itr != itrEnd && *itr < nTabCount; ++itr)
   3742                 SetWidthOrHeight( sal_True, 1,nCols, *itr, SC_SIZE_VISOPT, STD_EXTRA_WIDTH, false, sal_True);
   3743                 SetWidthOrHeight( false,1,nRows, *itr, SC_SIZE_VISOPT, 0, false, false);
   3744                 rDocShell.PostPaint( 0,0,*itr, MAXCOL,MAXROW,*itr,
   3745                                 PAINT_GRID | PAINT_LEFT | PAINT_TOP );

Should the for loop include all the block (until line 3745) or something should be done?

2) It could be linked too
cppcheck detected this:
[sc/source/core/tool/autoform.cxx:749] -> [sc/source/core/tool/autoform.cxx:752]: (performance) Variable 'bRet' is reassigned a value before the old one has been used.
[sc/source/core/tool/autoform.cxx:1019] -> [sc/source/core/tool/autoform.cxx:1029]: (performance) Variable 'bRet' is reassigned a value before the old one has been used.
[sc/source/core/tool/autoform.cxx:1082] -> [sc/source/core/tool/autoform.cxx:1092]: (performance) Variable 'bRet' is reassigned a value before the old one has been used.
[sc/source/core/tool/autoform.cxx:1107] -> [sc/source/core/tool/autoform.cxx:1110]: (performance) Variable 'bRet' is reassigned a value before the old one has been used.

In some of these cases, the result of a test is retrieved in bRet but unused even if there's a problem.

Any idea?

Julien
John LeMoyne Castle John LeMoyne Castle
Reply | Threaded
Open this post in threaded view
|

Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx

Julien,

Looks to me like a very nice catch ...

Here is the version as of 2011.06.15 showing braces on the for loops in both then and else blocks...
from open grok  --
http://opengrok.libreoffice.org/xref/core/sc/source/ui/docshell/docfunc.cxx?r=1b363f632110e80ead67ff376e92e4487556ca55

   3735
   3736         if (bSize)
   3737         {
   3738             SCCOLROW nCols[2] = { nStartCol, nEndCol };
   3739             SCCOLROW nRows[2] = { nStartRow, nEndRow };
   3740
   3741             for (SCTAB nTab=0; nTab<nTabCount; nTab++)
   3742                 if (aMark.GetTableSelect(nTab))
   3743                 {
   3744                     SetWidthOrHeight( sal_True, 1,nCols, nTab, SC_SIZE_VISOPT, STD_EXTRA_WIDTH, false, sal_True);
   3745                     SetWidthOrHeight( false,1,nRows, nTab, SC_SIZE_VISOPT, 0, false, false);
   3746                     rDocShell.PostPaint( 0,0,nTab, MAXCOL,MAXROW,nTab,
   3747                                     PAINT_GRID | PAINT_LEFT | PAINT_TOP );
   3748                 }
   3749         }
   3750         else
   3751         {
   3752             for (SCTAB nTab=0; nTab<nTabCount; nTab++)
   3753                 if (aMark.GetTableSelect(nTab))
   3754                 {
   3755                     sal_Bool bAdj = AdjustRowHeight( ScRange(nStartCol, nStartRow, nTab,
   3756                                                         nEndCol, nEndRow, nTab), false );
   3757                     if (bAdj)
   3758                         rDocShell.PostPaint( 0,nStartRow,nTab, MAXCOL,MAXROW,nTab,
   3759                                             PAINT_GRID | PAINT_LEFT );
   3760                     else
   3761                         rDocShell.PostPaint( nStartCol, nStartRow, nTab,
   3762                                             nEndCol, nEndRow, nTab, PAINT_GRID );
   3763                 }
   3764         }

The next version (2011.07.05)
http://opengrok.libreoffice.org/xref/core/sc/source/ui/docshell/docfunc.cxx?r=2b88f6d32f572792597ccbb15276b9db52db7d10
is a commit labelled "change remaining manual loops to ScMarkData::iterator " and the braces disappear on just one of the loops.  Because the braces were once there and, especially because the else block has ~ same action within the braces, I say that they should definitely come back.  Even though, I am not at all clear what this code is doing (whilst autoformatting selected cells in Calc)... this is an obvious oopsie within a greater code cleanup that should be corrected.  

This is not fdo# 47466 --  that crash gets close but doesn't quite get to this code: that non-reproducible crash occurred inside the AutoFormat function called just above the missing braces.

After some more looking at the code, it appears to be applying widths and heights... Some testing shows that if one selects a tabular area and apply a non-default width and height (Format->Rows/Columns->Height/Width) then and uncheck AutoFormat Width and Height under AutoFormat... -> More.  Doing the AutoFormat then forgets the width (doesn't loop) but not the height (still loops).  

The block of code in question starts to look like maybe it is fix code that corrects an overactive AutoFormat function called just above.  

This here bug  --  https://bugs.freedesktop.org/show_bug.cgi?id=51524
Shows similar loss-of-width behavior in collapse/uncollapse (as opposed to straight up AutoFormat)

There are also several reports of loss of height information (including self-healing upon edit) - most of them are closed &/or related to file open/close.  Maybe this block of W/H fix code is missing elsewhere, maybe it is not needed here anymore, maybe the open & save fix the values better than this now, maybe this simply is the width/height portion of the AutoFormat ... hmmmm
 
I will check to see if inserting braces in the code fixes whatever loss-of-width/height behavior that I can  reproduce or find.  

My 2cents... and now a couple more:

I note that, for all its type mushiness, BASIC will not allow this kind of mistake.  The then-clause must either be all on one line (can do IF test THEN StmtA : StmtB : StmtC) and a multi-line then-clause must be terminated with End If or Else.

Julien, I think you should get the patch credit - this is your find.   However, if it will help you continue to run cppchecks and find more of these interesting puzzles, I would be happy to patch a pair of braces for you and mark the appropriate bugs as resolved by you ;-D
 
Sweet catch,
LeMoyne

John LeMoyne Castle John LeMoyne Castle
Reply | Threaded
Open this post in threaded view
|

Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx

If only I could *always* read well  ... and I didn't second guess myself so easily ...

The stack trace from 47466 - Crash occurring in Windows 64bit versions...

 msvcr90!_invalid_parameter_noinfo+0xc
 sclo!std::_Tree<std::_Tset_traits<short,std::less<short>,std::allocator<short>,0> >::const_iterator::operator*+0x35 [c:\program files\microsoft visual studio 9.0\vc\include\xtree @ 264]
 sclo!std::_Tree<std::_Tset_traits<short,std::less<short>,std::allocator<short>,0> >::iterator::operator*+0xf [c:\program files\microsoft visual studio 9.0\vc\include\xtree @ 466]
 sclo!ScDocFunc::AutoFormat+0x520 [d:\losrc\3553\sc\source\ui\docshell\docfunc.cxx @ 3739]
 sclo!ScViewFunc::AutoFormat+0x70 [d:\losrc\3553\sc\source\ui\view\viewfun2.cxx @ 1575]
 
Is almost identical to yours.  

You HAVE reproduced fdo#47466.

The iterator functions appear to paper over the access at itr.end() everywhere except 1) Windows 64bit where it crashes and 2) your cppcheck which has replaced the standard iterator with a safe iterator, if I am not mistaken.   But really the unasked for width adjustments people see could be the result of an uncaught exception shortcutting all the rest of the width/height fix, etc so format does not get restored to fixed width and height during the AutoFormat.   Bubbling exception from this could also help explain the report of related undo failures after the errant width/height change as in
fdo# 34552   EDITING: Calc loses row height value when modifying a cell  
https://bugs.freedesktop.org/show_bug.cgi?id=34552 
 -- if it skips out from the point of the missing braces then it will fail to record the undo info for the width height change and that could cause the behavior there

And maybe even other bugs if calls to this AutoFormat function are involved there ...
Like   https://bugs.freedesktop.org/show_bug.cgi?id=57176
Undo of delete of Conditional Formatting works for one cell but fails for multi-cell (cf. if (bSize) ...)

On final note, I don't think that
bool ScDocFunc::AutoFormat( const ScRange& rRange, ...
will return bSuccess = true under any circumstances.  Just sayin' .... ;-)

I will seek more bugs and test them before and after the brace insert in the next few days ...  

Again... looks like a real good catch here.  Gratz and thank you for the puzzle.

Happy New Year !!!
LeMoyne


julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx

Thank you John for your investigation about all this.

In fact, I was leafing through some cppcheck reports.
To simplify,  I pushed some slight fixes  in sc/source/core/tool/autoform.cxx (see http://cgit.freedesktop.org/libreoffice/core/commit/?id=2e1abe1c59b1121ffb5d46afe82ce985cb70c4db).
So now we've got this:
[sc/source/core/tool/autoform.cxx:1102] -> [sc/source/core/tool/autoform.cxx:1105]: (performance) Variable 'bRet' is reassigned a value before the old one has been used.

   1102         bRet = (rStream.GetError() == 0);
   1103         //-----------------------------------------------------------
   1104         rStream << (sal_uInt16)(maData.size() - 1);
   1105         bRet = (rStream.GetError() == 0);
   1106         MapType::iterator it = maData.begin(), itEnd = maData.end();
   1107         for (++it; bRet && it != itEnd; ++it) // Skip the first item.
   1108             bRet = it->second->Save(rStream, fileVersion);

So bRet value of 1102 isn't taken into account since there's another assignation line 1105.
But just after, I noticed that at line 1107 "it" variable was increased without checking it != itEnd
So I checked bugs concerning autoformats and ran into fdo#47466.
I gave a try to it by adding an "if (it != itEnd)" before line 1107 and finally had the bt of my first message.

So I looked at the git history as you indicated and found about the missing braces was due to a slight mistake during a cleaning.

I pushed a fix (see http://cgit.freedesktop.org/libreoffice/core/commit/?id=f843850ee9994653673ef5591aae875d7fb22fed).

Now there are still the cppcheck report + the potential lacking of iterator test before loop

Julien
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx

In reply to this post by John LeMoyne Castle
BTW, it's not cppcheck which had safe iterator since it just allow to scan sources.
I rather think it's "--enable-dbgutil" in autogen.lastrun which triggered safe iterator.

Julien
Lubos Lunak Lubos Lunak
Reply | Threaded
Open this post in threaded view
|

Re: Wrong indentation which leads to segfault in sc/source/ui/docshell/docfunc.cxx

In reply to this post by John LeMoyne Castle
On Tuesday 01 of January 2013, John LeMoyne Castle wrote:
> Julien,
>
> Looks to me like a very nice catch ...
...

> The next version (2011.07.05)
> http://opengrok.libreoffice.org/xref/core/sc/source/ui/docshell/docfunc.cxx
>?r=2b88f6d32f572792597ccbb15276b9db52db7d10 is a commit labelled
> &quot;change remaining manual loops to
> ScMarkData::iterator &quot; and the braces disappear on just one of the
> loops.  Because the braces were once there and, especially because the else
> block has ~ same action within the braces, I say that they should
> definitely come back.  Even though, I am not at all clear what this code is
> doing (whilst autoformatting selected cells in Calc)... this is an obvious
> oopsie within a greater code cleanup that should be corrected.
...
> I note that, for all its type mushiness, BASIC will not allow this kind of
> mistake.  The then-clause must either be all on one line (can do IF test
> THEN StmtA : StmtB : StmtC) and a multi-line then-clause must be terminated
> with End If or Else.

http://tinderbox.libreoffice.org/cgi-bin/gunzip.cgi?tree=MASTER&full-log=1356992414.21678#20458 :

20458         In file included from <stdin>:1:
20459         /home/tinderbox/clang-master-build/sc/source/ui/docshell/docfunc.cxx:3743:17:
warning: statement aligned as second statement in for body but not in a
statement block [loplugin]
20460                         SetWidthOrHeight( false,1,nRows, *itr,
SC_SIZE_VISOPT, 0, false, false);
20461                         ^
20462         /home/tinderbox/clang-master-build/sc/source/ui/docshell/docfunc.cxx:3742:17:
note: for body statement is here [loplugin]
20463                         SetWidthOrHeight( sal_True, 1,nCols, *itr,
SC_SIZE_VISOPT, STD_EXTRA_WIDTH, false, sal_True);
20464                         ^
20465         1 warning generated.

--
 Lubos Lunak
 [hidden email]
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice