Bug 138691 - Crash when trying to save a form using an image bigger than length of Binary fix field
Summary: Crash when trying to save a form using an image bigger than length of Binary ...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
7.2.0.0.alpha0+
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:7.4.0
Keywords:
Depends on:
Blocks: Crash
  Show dependency treegraph
 
Reported: 2020-12-06 11:12 UTC by Julien Nabet
Modified: 2022-10-19 15:21 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
bt with debug symbols (7.72 KB, text/plain)
2020-12-06 11:12 UTC, Julien Nabet
Details
gdb bt (12.77 KB, text/plain)
2020-12-06 12:20 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Nabet 2020-12-06 11:12:07 UTC
Description:
On pc Debian x86-64 with master sources updated today, I got a crash when trying to save a form using an image bigger than length of Binary fix field.

Steps to Reproduce:
1. Launch Base
2. Create Firebird embedded file (enable experimental features for prerequisite) or open an existing Firebird embedded file (no need to enable experimental features in this case)
3. Create a simple table TABLE1 with:
- id/INTEGER primary key
- f1/Binary (fix) BINARY (put 100 for length)

4. Create a form with TABLE1 with wizard:
 4.1 : select the brand new table and use both fields + Next
 4.2 : "Decide if you want to set up a subform" => Next
 4.3 : "Arrange the controls on your form" => Select the second one from left of the first row + Finish
=> the form displays and you can enter data
5. type "1" in first field
6. right click on second field => "Insert Image from..." appears, select it
7. select an image of a length > 100
8. close form => dialog box asks to save
9. click "Yes"

Actual Results:
Crash

Expected Results:
No crash


Reproducible: Always


User Profile Reset: No



Additional Info:
With HSQLDB embedded, it's more difficult to reproduce this since by default length for Binary (fix) BINARY is 2147483647 and you can't change it compared with Firebird embedded database.
Comment 1 Julien Nabet 2020-12-06 11:12:43 UTC
Created attachment 167864 [details]
bt with debug symbols
Comment 2 Julien Nabet 2020-12-06 11:20:59 UTC
Robert: would you have some minutes to reproduce this?

Lionel: taking a look at the bt, the pb is here:
   1847             case DataType::BINARY:
   1848             case DataType::VARBINARY:
   1849             case DataType::LONGVARBINARY:
   1850             case DataType::BLOB:
   1851                 {
   1852                     Any x(_rValue.makeAny());
   1853                     Sequence< sal_Int8> aBytes;
   1854                     if(x >>= aBytes)
   1855                         _xParams->setBytes(parameterIndex,aBytes);
See https://opengrok.libreoffice.org/xref/core/connectivity/source/commontools/dbtools.cxx?r=d6d80c4e#1847

after x >>= aBytes, aBytes is just a sequence of 0 because we get over max size.
So setBytes is called with a 0 size sequence and it crashes.

A simple fix would be to change "if" condition to also test that aBytes is not a 0 length sequence.
Pb is the user isn't warned about the pb.

Another fix would be to add a specific test length and create a new string message here:
connectivity/inc/strings.hrc
like:
"The image is too big compared for the field." (it can be greatly improved I suppose, it's just to give an idea).

I'm pretty sureit's not specific to Firebird but with HSQLDB, as indicated in initial description, length of the Binary (fix) BINARY is 2147483647 and it's readonly (I didn't investigate why).

Remark: I don't know how "Any" => "Sequence" conversion takes length into account to retrieve 0 size when the image is too big.
Comment 3 Julien Nabet 2020-12-06 11:25:54 UTC
Sorry Robert, you won't be able to test this (at least with Firebird) for the moment,  https://gerrit.libreoffice.org/c/core/+/107280 is a prerequisite.
Comment 4 Lionel Elie Mamane 2020-12-06 11:54:17 UTC
(In reply to Julien Nabet from comment #2)

> after x >>= aBytes, aBytes is just a sequence of 0 because we get over max
> size.
> So setBytes is called with a 0 size sequence and it crashes.

I think that for robustness of LibreOffice, that would be useful to fix that (namely, that it doesn't crash when called with empty sequence).

> Remark: I don't know how "Any" => "Sequence" conversion takes length into
> account to retrieve 0 size when the image is too big.

Does the "Any" actually contain something not of zero size?
Comment 5 Julien Nabet 2020-12-06 12:20:43 UTC
Created attachment 167865 [details]
gdb bt

Argh, it still crashes but now it's not an empty sequence.
Still, since the image size I chose on purpose is bigger than the length of the field, the crash was expected.
I hate this kind of "Heisenberg" behavior but now at least, the error is understandeable.
Now I suppose a check of size (0 size or size bigger than length of the field) should be done in common part and not Firebird specific.
Comment 6 Julien Nabet 2020-12-06 12:56:26 UTC
I kept on the tests and I finally could reproduce the empty sequence too:
Here are the steps:
- create a table following steps for initial description but with length 10000 (instead of 100)
- create a form the same way as initial description
- in the form
- type "1" for field 1
- choose image with a length < 10000
- close form and save
- reopen the form
- choose image with a length > 10000
=> crash with an empty sequence.

So not Heisenberg at all! :-)

Now let's find the possible and most appropriate location to fix (check+warning) this between:
- forms/source/runtime/formoperations.cxx
- forms/source/component/DatabaseForm.cxx
- dbaccess/source/core/api/RowSet.cxx
- dbaccess/source/core/api/RowSetCache.cxx
- dbaccess/source/core/api/KeySet.cxx
- dbaccess/source/core/api/CacheSet.cxx
- connectivity/source/commontools/dbtools.cxx
- dbaccess/source/core/api/preparedstatement.cxx

I removed connectivity/source/drivers/firebird/PreparedStatement.cxx since we want a generic fix (not Firebird specific).
Comment 7 Julien Nabet 2020-12-06 14:23:13 UTC
Since we must deal with update + insert case, I think connectivity/source/commontools/dbtools.cxx should be used.
Now the problem is how to retrieve precision (aka max length) of a column...

I tested with a try catch (Exception or RuntimeException), LO doesn't care and crashes also in this case.

=> I'm stuck now
Comment 8 Xisco Faulí 2020-12-07 09:07:18 UTC
I think it's fair to move it to NEW
Comment 9 Julien Nabet 2021-11-01 17:51:09 UTC
Just thinking loudly, perhaps we should check image size just after having selected it?
It's in OImageControlControl::implInsertGraphics (see https://opengrok.libreoffice.org/xref/core/forms/source/component/ImageControl.cxx?r=1875b3d9&mo=23636&fi=762#762)
If the image size is > field size => error message and the action isn't done.
Comment 10 Robert Großkopf 2021-11-01 17:55:54 UTC
(In reply to Julien Nabet from comment #9)
> Just thinking loudly, perhaps we should check image size just after having
> selected it?
> It's in OImageControlControl::implInsertGraphics (see
> https://opengrok.libreoffice.org/xref/core/forms/source/component/
> ImageControl.cxx?r=1875b3d9&mo=23636&fi=762#762)
> If the image size is > field size => error message and the action isn't done.

I would prefer this solution.
Comment 11 Commit Notification 2022-02-05 16:27:02 UTC
Mike Kaganski committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/94ba3770ffe31bd26e0c67a5609c8935994b808a

tdf#138691: avoid buffer 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 12 Mike Kaganski 2022-02-05 20:16:35 UTC
(In reply to Robert Großkopf from comment #10)
> (In reply to Julien Nabet from comment #9)
> > perhaps we should check image size just after having selected it?
> > It's in OImageControlControl::implInsertGraphics ...
> > If the image size is > field size => error message and the action isn't done.
> 
> I would prefer this solution.

But can't forms pre-process data before inserting into tables? Then isn't checking something at entry time too soon?
Comment 13 Robert Großkopf 2022-02-06 07:57:51 UTC
(In reply to Mike Kaganski from comment #12)
> (In reply to Robert Großkopf from comment #10)
> > (In reply to Julien Nabet from comment #9)
> > > perhaps we should check image size just after having selected it?
> > > It's in OImageControlControl::implInsertGraphics ...
> > > If the image size is > field size => error message and the action isn't done.
> > 
> > I would prefer this solution.
> 
> But can't forms pre-process data before inserting into tables? Then isn't
> checking something at entry time too soon?

Forms will only link from the path of the image to the table of the database. It works like this:
oSimpleFileAccess = createUnoService("com.sun.star.ucb.SimpleFileAccess")
oStream = oSimpleFileAccess.openFileRead(stUrl)
oField.BoundField.updateBinaryStream(oStream, oStream.getLength())

There is no possibility to limit the content for this field in the form.
Comment 14 Mike Kaganski 2022-02-06 09:06:54 UTC
(In reply to Robert Großkopf from comment #13)

I'm afraid I don't quite understand you. There are form events, including "Before Submitting"; do you say I can't replace whatever value I want with some processed data, e.g. with a processed (reduced) thumbnail of what I loaded into the control?
Comment 15 Robert Großkopf 2022-02-06 09:51:57 UTC
(In reply to Mike Kaganski from comment #14)
> (In reply to Robert Großkopf from comment #13)
> 
> I'm afraid I don't quite understand you. There are form events, including
> "Before Submitting"; do you say I can't replace whatever value I want with
> some processed data, e.g. with a processed (reduced) thumbnail of what I
> loaded into the control?

The form control doesn't support any limit for the content, which should be read.
But: If I will control it by macro I could say:
oStream.getLength()
is too big for the field. So I could stop submitting.

I haven't tested much with images and forms, special with images, which should be saved in a table of the database. I won't do this, because it expands the database too much. I prefer to link images by the image control to the database.
Comment 16 Mike Kaganski 2022-02-06 10:15:13 UTC
(In reply to Robert Großkopf from comment #15)

Ok, so this means:

1. You shouldn't implement comment 12, since there may be a different workflow compared to what you prefer.
2. The control could benefit from a new optional property "max size". That could be possibly set automatically upon creation of the control on a fixed-size database field.