Bug 146795 - Calc crashes when pasting data into filtered cells (mdds)
Summary: Calc crashes when pasting data into filtered cells (mdds)
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
7.3.0.0.alpha1+
Hardware: All Linux (All)
: high major
Assignee: Kohei Yoshida
URL:
Whiteboard: target:7.4.0 target:7.3.1
Keywords: bibisected, bisected, haveBacktrace, regression
Depends on:
Blocks: AutoFilter
  Show dependency treegraph
 
Reported: 2022-01-16 13:53 UTC by Kevin Suo
Modified: 2022-03-04 10:34 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments
test1.ods (9.57 KB, application/vnd.oasis.opendocument.spreadsheet)
2022-01-16 13:54 UTC, Kevin Suo
Details
gdb_backtrace.log (87.58 KB, text/plain)
2022-01-16 14:41 UTC, Kevin Suo
Details
bt_with_patch.txt (124.22 KB, text/plain)
2022-02-03 02:27 UTC, Kevin Suo
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Suo 2022-01-16 13:53:31 UTC
Steps to Reproduce:
1. Open attached test ods file.
2. Filter "11" in column b.
3. Type "a" in B2.
4. Ctrl+C in B2.
5. Click in B3, then Ctrl+Shift+Down.
6. Click "OK" at the override confirmation dialog.

-> Crash.

Version: 7.4.0.0.alpha0+ / LibreOffice Community
Build ID: d73602dc51aa8829fc88e5e67e2b0c4da6b8f715
CPU threads: 8; OS: Linux 5.15; UI render: default; VCL: gtk3
Locale: zh-CN (zh_CN.UTF-8); UI: zh-CN
Build Platform: Fedora34@X64, Branch:master, bibisect-linux-64-7.4-CN
Calc: threaded

Fedora 34, Gnome 40.5, Wayland.
Comment 1 Kevin Suo 2022-01-16 13:54:00 UTC
Created attachment 177574 [details]
test1.ods
Comment 2 Kevin Suo 2022-01-16 14:02:49 UTC
(In reply to Kevin Suo from comment #0)
Steps to Reproduce:
1. Open attached test ods file.
2. Filter "11" in column b.
3. Type "a" in B2.
4. Ctrl+C in B2.
5. Click in B3, then Ctrl+Shift+Down.
6. Ctrl+V. Click "OK" at the override confirmation dialog.

-> Crash.
Comment 3 Kevin Suo 2022-01-16 14:08:28 UTC
$ git log 0464b86787da269be7b16a6f1f124d774f78fa97..eb07a0e76fe240a184348d96a6cebf7c0a229ac0 --pretty=reference 
eb07a0e76fe2 (Upgrade mdds and liborcus to 2.0.0 and 0.17.0, respectively., 2021-11-01)
9b9f4a4487e9 (Update git submodules, 2021-11-03)
ff22a3ff99da (Add back `== 2`, 2021-11-03)
913c9aa8f5bb (Update git submodules, 2021-11-03)
29b39dfb667d (Update git submodules, 2021-11-03)
317100ad1d22 (Update git submodules, 2021-11-03)
0c67012a1886 (Update git submodules, 2021-11-03)
c587ac9e9449 (Update git submodules, 2021-11-03)
05a57cee0123 (Update git submodules, 2021-11-03)
5340ba24dda5 (Update git submodules, 2021-11-03)
cbe437ea1e5a (Update git submodules, 2021-11-03)
2aef034db210 (Update git submodules, 2021-11-03)
0481ed9e1381 (Update git submodules, 2021-11-03)
f6435fed1192 (Update git submodules, 2021-11-03)
6d93317ab62f (Update git submodules, 2021-11-03)
4ed4f18d62c1 (Update git submodules, 2021-11-03)
7fed1b6be186 (Update git submodules, 2021-11-03)
adea4657391d (Update git submodules, 2021-11-03)
02d8b43827ad (Update git submodules, 2021-11-03)
b64efd7b291c (Revert "loplugin:finalclasses", 2021-11-03)

Adding Kohei Yoshida to cc. Could you please take a look? Thanks.
Comment 4 Kevin Suo 2022-01-16 14:41:21 UTC
Created attachment 177575 [details]
gdb_backtrace.log
Comment 5 Xisco Faulí 2022-01-20 14:35:27 UTC
Reproduced in

Version: 7.4.0.0.alpha0+ / LibreOffice Community
Build ID: bd5492275d31f59b1d269205018d1487af52426f
CPU threads: 8; OS: Linux 5.10; UI render: default; VCL: gtk3
Locale: es-ES (es_ES.UTF-8); UI: en-US
Calc: threaded
Comment 6 Xisco Faulí 2022-01-20 14:38:39 UTC
I do confirm the issue was introduced by:

author	Kohei Yoshida <kohei@libreoffice.org>	2021-11-01 14:01:22 -0400
committer	Kohei Yoshida <kohei@libreoffice.org>	2021-11-03 21:17:18 +0100
commit eb07a0e76fe240a184348d96a6cebf7c0a229ac0 (patch)
tree 23ab960b7a163696e4a7c1d4c4c20c1340fa14b3
parent 9b9f4a4487e9ada1885d45a8b1ba0234a4a9fc26 (diff)
Upgrade mdds and liborcus to 2.0.0 and 0.17.0, respectively.

Bisected with: bibisect-linux64-7.3

@Kohei, Could you please take a look ?
Comment 7 Kohei Yoshida 2022-01-20 15:12:27 UTC
If someone can isolate this to a reproducible case with mdds alone, that would be helpful.
Comment 9 Xisco Faulí 2022-01-20 17:01:02 UTC
(In reply to Kohei Yoshida from comment #7)
> If someone can isolate this to a reproducible case with mdds alone, that
> would be helpful.

Hi Kohei,
Could you please elaborate? How can I know a case is reproducible in mdds only ?
Comment 10 Kohei Yoshida 2022-01-20 17:19:39 UTC
So, mdds is an independent library that provides C++ API.  Calc uses it to manage cell data I/O.  Any access patterns Calc does to mdds can be reduced to a sequence of C++ calls to mdds::multi_type_vector.  In this case, I'd like such reduced test case code to be provided to make it easier for me to look into this in mdds.

Otherwise, that would be the first thing I'd have to do: to reduce Calc's access pattern that triggers the crash to a series of C++ calls to mdds::multi_type_vector only so that I can use that as a future test case for mdds.  I need to do this for efficiency reasons; debugging Calc takes 10x more resources than debugging just mdds alone.

If that's not reasonable ask, then fine. But I can't guarantee a timely investigation.
Comment 11 Kevin Suo 2022-01-21 02:15:44 UTC
Kohei, is it possible to do bisect, on libreoffice level, into the mdds commits? I see the changebwas from 1.7.0 to 2.0.0 and there may not be many commits between that.
Comment 12 Kohei Yoshida 2022-01-21 02:20:49 UTC
(In reply to Kevin Suo from comment #11)
> Kohei, is it possible to do bisect, on libreoffice level, into the mdds
> commits? I see the changebwas from 1.7.0 to 2.0.0 and there may not be many
> commits between that.

No.
Comment 13 Aron Budea 2022-01-21 12:06:25 UTC
(In reply to Kohei Yoshida from comment #7)
> If someone can isolate this to a reproducible case with mdds alone, that
> would be helpful.
Kohei, could you add code pointers where mdds is called during the described steps? (or what mdds functions are called) I'd assume no one else reading this bug report has experience with that.
Comment 14 Kohei Yoshida 2022-01-21 23:26:56 UTC
(In reply to Aron Budea from comment #13)
> (In reply to Kohei Yoshida from comment #7)
> > If someone can isolate this to a reproducible case with mdds alone, that
> > would be helpful.
> Kohei, could you add code pointers where mdds is called during the described
> steps?

I don't have that info yet, but I can tell you how one may start investigating it.

Before you begin, it would be helpful to look through this page:

https://mdds.readthedocs.io/en/latest/multi_type_vector.html

to get an overview of what mdds::multi_type_vector does.  You don't have to read the entire page, but just going through the Quickstart section should give you a good idea of how to use mdds::multi_type_vector standalone.  The page also have API reference toward the bottom.

People here casually refers to mdds::multi_type_vector as just "mdds", but technically that's not correct.  mdds contains multiple data structures that Calc uses, and multi_type_vector is just one of them.  mdds::flat_segment_tree is another one Calc uses, but I won't go into that here.

ScColumn::maCells data member is where all cell values are stored, so tracing its use would be the starting point.

Here

https://opengrok.libreoffice.org/s?refs=maCells&project=core

shows all call sites of ScColumn::maCells which is quite extensive, understandably so since lots and lots of Calc code does access it since it's the core of Calc cell data I/O.

Now, the entry point for pasting data into Calc can be tricky since there are several entry points depending on where the data being pasted is coming from. But I believe it's ScViewFunc:PasteFromClip()

https://opengrok.libreoffice.org/xref/core/sc/source/ui/view/viewfun3.cxx?r=9436f7e2#871

for copy-n-pasting within the same Calc document.

The bMarkIsFiltered flag on line 1024 is set, since the value is being pasted onto a filtered area i.e. autofilter is applied in the destination area.

Eventually the code here will find its way into ScDocument::CopyToDocument, which moves to ScTable (sheet), and then eventually to ScColumn (column storage), and you can inspect how that code accesses and updates the maCells there...

That's the gist of it.

The tricky part is that, every action creates an undo object which also has its own ScDocument store, so that could be another source of the problem too. I just don't know where the problem may be...
Comment 15 Kohei Yoshida 2022-01-21 23:34:16 UTC
(In reply to Kohei Yoshida from comment #14)

> Eventually the code here will find its way into ScDocument::CopyToDocument,

Actually ScDocument::CopyFromClip().
Comment 16 Kohei Yoshida 2022-01-22 01:41:33 UTC
Ok, so in this case, the code eventually ends up in ScColumn::CopyOneCellFromClip(), which gets called twice, and crashes on the 2nd time.
Comment 17 Kohei Yoshida 2022-01-27 23:49:50 UTC
Actually I'm working on adding an optional tracer to mdds::multi_type_vector

https://gitlab.com/mdds/mdds/-/issues/70

With this, getting the above call info will be a bit easier.

I'm also investigating into the issue on the Calc side. But it's going very slow.
Comment 18 Kevin Suo 2022-01-28 07:31:53 UTC
Is downgrading the LO mdds version an option, in case it will take too long for a fix, provided that 7.3 is going to be released as "stable" to the public?
Comment 19 Kohei Yoshida 2022-01-28 22:52:47 UTC
The short story is that the issue is on the Calc side. The crash is caused by the uses of invalid position hints when modifying the cell store.  Sorting that out on the Calc side will fix this issue.

Having said that, the issue existed from at least 2014, but somehow it never manifested itself as a crash using the old storage layout of mdds::multi_type_vector (mtv).  In 2.0, mtv switched to an alternative layout, and somehow that triggers the crash from invalid position hints more easily.  No idea why that is.

I have a proof-of-concept fix that fixes the crash, but unfortunately that involves quite a bit of non-trivial change in Calc's CopyFromClip* code which may potentially introduce other regressions if not done right.  So I would still need to flesh out my fix there, which may take a while.
Comment 20 Kohei Yoshida 2022-01-29 00:15:38 UTC
(In reply to Kevin Suo from comment #18)
> Is downgrading the LO mdds version an option, in case it will take too long
> for a fix, provided that 7.3 is going to be released as "stable" to the
> public?

From my POV that makes little sense. But I'll leave that up to TDF.
Comment 21 Xisco Faulí 2022-01-29 11:17:03 UTC
(In reply to Kevin Suo from comment #18)
> Is downgrading the LO mdds version an option, in case it will take too long
> for a fix, provided that 7.3 is going to be released as "stable" to the
> public?

When possible, it's always better to have a fix rather than a revert.
Comment 22 Kohei Yoshida 2022-02-02 03:30:57 UTC
FYI my proposed fix is here: https://gerrit.libreoffice.org/c/core/+/129267

Two buildbots fail, but I'm having a hard time reproducing the test failures in my environment.
Comment 23 Kevin Suo 2022-02-03 02:27:02 UTC
Created attachment 178003 [details]
bt_with_patch.txt

I can reproduce the fail in testTdf92963 with your proposed patch. Error: attempt to copy from a singular iterator.

Fedora 34 x64. dbgutil build with "CC=clang" and "CXX=clang++" autogen.sh options enabled.

Attached is the gdb "bt" and "bt full".
Comment 24 Kevin Suo 2022-02-03 02:41:14 UTC
> I can reproduce the fail in testTdf92963 with your proposed patch.

Manually, With the proposed patch, Calc crashes when do the following:
1. Open sc/qa/unit/uicalc/data/tdf92963.ods.
2. Select cell range A1:C4, copy.
3. Select cell range A1:C1, paste.
--> Crash.
Comment 25 Kohei Yoshida 2022-02-03 04:47:26 UTC
(In reply to Kevin Suo from comment #24)
> > I can reproduce the fail in testTdf92963 with your proposed patch.
> 
> Manually, With the proposed patch, Calc crashes when do the following:
> 1. Open sc/qa/unit/uicalc/data/tdf92963.ods.
> 2. Select cell range A1:C4, copy.
> 3. Select cell range A1:C1, paste.
> --> Crash.

Yup, I can finally reproduce this locally.  Thanks!

I'm slowly starting to understand what's going on...  It's still caused by invalid position hints on the Calc side, but I'm still trying to figure out why that happens.
Comment 26 Kohei Yoshida 2022-02-03 04:52:27 UTC Comment hidden (no-value)
Comment 27 Kohei Yoshida 2022-02-03 04:53:47 UTC Comment hidden (no-value)
Comment 28 Kohei Yoshida 2022-02-03 04:59:19 UTC
The latest crash is very much in the same league as tdf#100852.  Even the stack trace looks very similar.

I curse Bugzilla's immutable comments.
Comment 29 Commit Notification 2022-02-04 01:41:30 UTC
Kohei Yoshida committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/703fb7739a5e604d90e147db6f67917b8d141150

tdf#146795: Make sure to use valid position hints to avoid crash.

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 30 Kohei Yoshida 2022-02-04 02:00:37 UTC
backport for libreoffice-7-3: https://gerrit.libreoffice.org/c/core/+/129267
Comment 31 Kevin Suo 2022-02-04 02:06:27 UTC
I can verify that it is now fixed on master. Thank you Kohei!
Comment 32 Commit Notification 2022-02-04 17:17:28 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/9f0d27c973323f522a4ca2bce557e1c095d32f77

tdf#146795: sc_uicalc: Add unittest

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 33 Kohei Yoshida 2022-02-07 22:08:25 UTC
(In reply to Kohei Yoshida from comment #30)
> backport for libreoffice-7-3: https://gerrit.libreoffice.org/c/core/+/129267

*sigh*

This is the right URL: https://gerrit.libreoffice.org/c/core/+/129478

Bugzilla is only for the smart people who never make mistakes...
Comment 34 Commit Notification 2022-02-10 15:23:11 UTC
Kohei Yoshida committed a patch related to this issue.
It has been pushed to "libreoffice-7-3":

https://git.libreoffice.org/core/commit/ea02ca06de604370061a59dd3084f7cfacccbe39

tdf#146795: Make sure to use valid position hints to avoid crash.

It will be available in 7.3.1.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.