Bug 147014 - FILEOPEN XLSX: Images anchored to cells are missing
Summary: FILEOPEN XLSX: Images anchored to cells are missing
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
6.0.0.3 release
Hardware: All All
: medium normal
Assignee: Aron Budea
URL:
Whiteboard: target:7.4.0 target:7.3.3 target:7.2.7
Keywords: bibisected, bisected, filter:xlsx
: 121074 (view as bug list)
Depends on:
Blocks: XLSX-Images
  Show dependency treegraph
 
Reported: 2022-01-27 03:18 UTC by Kevin Suo
Modified: 2022-03-28 18:55 UTC (History)
8 users (show)

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


Attachments
testImageAnchoredToCells.xlsx (2.39 MB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2022-01-27 03:18 UTC, Kevin Suo
Details
MSO2010Screenshot.png (89.43 KB, image/png)
2022-01-27 03:20 UTC, Kevin Suo
Details
Screenshot with LO 7.0.6 on Windows 10 (196.90 KB, image/png)
2022-01-27 10:37 UTC, Ming Hua
Details
XLSX created from scratch (118.93 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2022-01-31 03:18 UTC, Aron Budea
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Suo 2022-01-27 03:18:47 UTC
Created attachment 177817 [details]
testImageAnchoredToCells.xlsx

The attached test document contains png images anchored to cells in column B. When open with Calc, the images are missing. These images do show up when open this file with MSO 2010.

Steps to Reproduce:
1. Open the attached test document.

Current Result:
Images anchored to cells in column B are missing.

Expected Result:
There should be three images anchored to cells in column B.

Terminal Output:
warn:xmloff:15203:15203:sax/source/fastparser/fastparser.cxx:1328: unknown element xsi:type http://www.w3.org/2001/XMLSchema-instance
warn:xmloff:15203:15203:sax/source/fastparser/fastparser.cxx:1328: unknown element xsi:type http://www.w3.org/2001/XMLSchema-instance
warn:xmloff:15203:15203:sax/source/fastparser/fastparser.cxx:1252: unknown attribute defaultSlicerStyle=SlicerStyleLight1
warn:legacy.osl:15203:15203:sc/source/filter/oox/worksheethelper.cxx:535: WorksheetGlobals::getDrawPageSize - called too early, size invalid
warn:legacy.osl:15203:15203:sc/source/filter/oox/worksheethelper.cxx:535: WorksheetGlobals::getDrawPageSize - called too early, size invalid
warn:legacy.osl:15203:15203:sc/source/filter/oox/worksheethelper.cxx:535: WorksheetGlobals::getDrawPageSize - called too early, size invalid

Version: 7.4.0.0.alpha0+ / LibreOffice Community
Build ID: b9039e511ed103814dd3c2987c2e408aebb58058
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.

Also reproducible on the "oldest" of the linux-64-6.1 bibisect repo.

This problem was initially reported by a user in our LibreOffice Simplified Chinese discussion QQ chat group. The user sent us a business proprietary file, and the attached is the one I edited with MSO 2010 to simplify and to have sensitive information removed.
Comment 1 Kevin Suo 2022-01-27 03:20:19 UTC
Created attachment 177818 [details]
MSO2010Screenshot.png
Comment 2 Xisco Faulí 2022-01-27 09:20:26 UTC
Reproduced in

Version: 7.4.0.0.alpha0+ / LibreOffice Community
Build ID: 2fe5fcfec107be93793f77f868763ab58da3573a
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 3 Ming Hua 2022-01-27 10:32:48 UTC
(In reply to Kevin Suo from comment #0)
> Created attachment 177817 [details]
> testImageAnchoredToCells.xlsx

This sanitized file behaves the same as the original for me on Windows, i.e. a regression from 7.0 to 7.3.

Reproducible (images not displayed) with 7.3.0 RC1 and 7.4/master daily:
Version: 7.3.0.1 (x64) / LibreOffice Community
Build ID: 840fe2f57ae5ad80d62bfa6e25550cb10ddabd1d
CPU threads: 2; OS: Windows 10.0 Build 19043; UI render: Skia/Raster; VCL: win
Locale: zh-CN (zh_CN); UI: zh-CN
Calc: threaded
and
Version: 7.4.0.0.alpha0+ (x64) / LibreOffice Community
Build ID: eb69767d7c1bb8e6e780fd9503f08c9d7f5ecb45
CPU threads: 2; OS: Windows 10.0 Build 19043; UI render: Skia/Raster; VCL: win
Locale: zh-CN (zh_CN); UI: zh-CN
Calc: threaded

However NOT reproducible (images display like MSO) with 7.0.6:
Version: 7.0.6.2 (x64)
Build ID: 144abb84a525d8e30c9dbbefa69cbbf2d8d4ae3b
CPU threads: 2; OS: Windows 10.0 Build 19043; UI render: default; VCL: win
Locale: zh-CN (zh_CN); UI: en-US
Calc: threaded

Regression on Windows?  Can someone able to bibisect on Windows give this a try?
Comment 4 Ming Hua 2022-01-27 10:37:30 UTC
Created attachment 177826 [details]
Screenshot with LO 7.0.6 on Windows 10
Comment 5 Kevin Suo 2022-01-27 14:13:50 UTC
bibisectRequest on Windows.
Comment 6 Aron Budea 2022-01-30 06:16:01 UTC
Looks like this started with the following commit on Windows, bibisected using repo bibisect-win64-7.1.

https://cgit.freedesktop.org/libreoffice/core/commit/?id=3d90997fb6f232d8008df4d166d7b97b869c200f
author		Noel Grandin <noel@peralex.com>	2020-10-28 08:30:36 +0200
committer	Noel Grandin <noel.grandin@collabora.co.uk>	2020-11-11 06:34:17 +0100

make tools::Long 64-bit on Windows platform
Comment 7 Kevin Suo 2022-01-30 08:31:40 UTC Comment hidden (obsolete)
Comment 8 Kevin Suo 2022-01-30 08:32:19 UTC Comment hidden (obsolete)
Comment 9 Aron Budea 2022-01-31 03:18:46 UTC
Created attachment 177926 [details]
XLSX created from scratch

Note that this issue isn't directly caused by the referenced commit.

The problem is an overflow in when getting the Size property of a cell range spanning more than a million rows, height is negative here:
https://opengrok.libreoffice.org/xref/core/sc/source/filter/oox/worksheethelper.cxx?r=75036ee9#1361

The overflow itself happens here, when sizes of type __int64 are converted into long, because the components of awt::Size are long:
https://opengrok.libreoffice.org/xref/core/sc/source/ui/unoobj/cellsuno.cxx?r=c3d5c9a0#5710

awt::Size:
https://api.libreoffice.org/docs/idl/ref/structcom_1_1sun_1_1star_1_1awt_1_1Size.html

It's fairly simple to create a bugdoc from scratch:
- Increase height to ~5x the normal size (select all cells, and resize one of the rows),
- Insert an image,
- Save as XLSX, then reload.

Eike, is the type of the Size property not being able to hold the size of a large cell range (eg. largest sheet size) a fundamental problem, and if so, what do you think could be done about it? Clamping the values would be a simple fix, but I'm worried about other potential issues with the insufficiently sized type.
Comment 10 Eike Rathke 2022-01-31 14:12:03 UTC
As
https://opengrok.libreoffice.org/xref/core/sc/source/filter/oox/worksheethelper.cxx?r=75036ee9#387
says, awt::Size maDrawPageSize is "Current size of the drawing page in 1/100 mm."

Clamping the setting of maDrawPageSize (and points to be accessed on it) might be an interim solution, unless a file actually uses the drawing layer more than 21km further down..

One could try to replace all awt::Point and awt::Size and awt::Rectangle usage in the filter with 64-bit counter parts to at least have the filter internals calculate on proper coordinates and clamp later, which would shift problems to another (though less likely) boundary.

Note however that attachment 177926 [details] and comment 9 is an *export* problem, the original report is an *import* problem. For the export we have the additional constraint that the Pos/Size properties obtained in cellsuno.cxx are part of the published UNO API and expected to be held by the awt::Point and awt::Size types. Clamping there is not so good, rather take care of negative 32-bit values when converting to 64-bit values to roll the overflow back.
Comment 11 Aron Budea 2022-02-07 14:31:12 UTC
(In reply to Eike Rathke from comment #10)
> One could try to replace all awt::Point and awt::Size and awt::Rectangle
> usage in the filter with 64-bit counter parts to at least have the filter
> internals calculate on proper coordinates and clamp later, which would shift
> problems to another (though less likely) boundary.
Thanks for the comments, Eike! Would there be a way to circumvent the UNO calls to avoid already getting a truncated page size/cell position?
 
> Note however that attachment 177926 [details] and comment 9 is an *export*
> problem, the original report is an *import* problem.
There could be an export problem as well, what I noticed with that sample is that it opens fine in an older version, but I didn't check if there are further issues.
Comment 12 Aron Budea 2022-02-12 08:34:22 UTC
Never mind, figured it out.
Comment 13 Commit Notification 2022-03-28 09:26:43 UTC
Aron Budea committed a patch related to this issue.
It has been pushed to "master":

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

tdf#147014 Image missing due to integer overflow

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 14 Aron Budea 2022-03-28 09:55:18 UTC
Fixed in trunk with a workaround, backport to 7.3 is on gerrit.

The real fix would be to use 64-bit types regardless whether build is 32-/64-bit (the convenient tools::Rectangle uses tools::Long, which isn't always 64-bit).
Comment 15 Xisco Faulí 2022-03-28 09:57:12 UTC Comment hidden (obsolete)
Comment 16 Xisco Faulí 2022-03-28 09:57:31 UTC
btw, thanks for fixing this issue!!
Comment 17 Aron Budea 2022-03-28 10:06:49 UTC
(In reply to Xisco Faulí from comment #15)
> Do you think this could be an easyhack ?
I don't know, Eike, Luboš, do you have an opinion on that?
Comment 18 Xisco Faulí 2022-03-28 10:08:28 UTC
(In reply to Xisco Faulí from comment #15)
> (In reply to Aron Budea from comment #14)
> > Fixed in trunk with a workaround, backport to 7.3 is on gerrit.
> > 
> > The real fix would be to use 64-bit types regardless whether build is
> > 32-/64-bit (the convenient tools::Rectangle uses tools::Long, which isn't
> > always 64-bit).
> 
> Do you think this could be an easyhack ?

Adding Hossein as well
Comment 19 Commit Notification 2022-03-28 12:04:40 UTC
Aron Budea committed a patch related to this issue.
It has been pushed to "libreoffice-7-3":

https://git.libreoffice.org/core/commit/4a614ad8c32ad09f4085980e49dce251358a0a46

tdf#147014 Image missing due to integer overflow

It will be available in 7.3.3.

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 20 Timur 2022-03-28 12:49:02 UTC
*** Bug 121074 has been marked as a duplicate of this bug. ***
Comment 21 Commit Notification 2022-03-28 18:55:31 UTC
Aron Budea committed a patch related to this issue.
It has been pushed to "libreoffice-7-2":

https://git.libreoffice.org/core/commit/483b96874a07dc2997367e5627f537686e92f851

tdf#147014 Image missing due to integer overflow

It will be available in 7.2.7.

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.