Bug Hunting Session
Bug 117446 - Firebird: Migration: LibO hangs with data type Binary[VARBINARY] OR Binary(fix)[BINARY] AND data in hsql column exceeds new column size ( 8000 bytes )
Summary: Firebird: Migration: LibO hangs with data type Binary[VARBINARY] OR Binary(fi...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
6.1.0.0.alpha1+
Hardware: All All
: high major
Assignee: Not Assigned
URL:
Whiteboard: target:6.2.0 target:6.1.0.1
Keywords:
Depends on:
Blocks: Database-Firebird-Migration
  Show dependency treegraph
 
Reported: 2018-05-05 21:06 UTC by Drew Jensen
Modified: 2018-06-09 16:51 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
test file with data (49.09 KB, application/vnd.oasis.opendocument.database)
2018-05-05 21:06 UTC, Drew Jensen
Details
Test file using binary(fix)[BINARY] (11.66 KB, application/vnd.oasis.opendocument.database)
2018-05-06 18:45 UTC, Drew Jensen
Details
valgrind trace (10.03 KB, application/x-bzip)
2018-05-16 20:07 UTC, Julien Nabet
Details
bt with debug symbols (13.05 KB, text/plain)
2018-05-16 20:50 UTC, Julien Nabet
Details
bt about length of varbinary on hsqldb (10.54 KB, text/plain)
2018-05-17 16:35 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Drew Jensen 2018-05-05 21:06:06 UTC
Description:
The migration routine declares table columns of type Binary[VARBINARY] from hsql into Text[VARCHAR] (according to Libo 6.0.3.2. 

If there is an image file stored in the first record this causes libo to become non-responsive and it must be forced to quit.

stdout reads: (soffice:26591): GLib-GObject-WARNING **: 16:52:47.499: ../../../../gobject/gsignal.c:3492: signal name 'selection_changed' is invalid for instance '0x30b9390' of type 'OOoAtkObjCompTxt'
double free or corruption (!prev)


Steps to Reproduce:
1. download test file, open it
2. open form say later when it asks to migrate
3. form displayes graphic from table in image control
4. close the file, reopen it
5  open the form, answer yes to migration prompt

Actual Results:  
system becomes non-responsive

Expected Results:
finish migration open form


Reproducible: Always


User Profile Reset: No



Additional Info:
tested w/build:
Version: 6.1.0.0.alpha1+
Build ID: f1579d3d6c5f5f3a651825e035b93bee7a4f43c6
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk2; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2018-05-03_10:04:51
Locale: en-US (en_US.UTF-8); Calc: group


User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/65.0.3325.181 Chrome/65.0.3325.181 Safari/537.36
Comment 1 Drew Jensen 2018-05-05 21:06:47 UTC
Created attachment 141910 [details]
test file with data

single table, one record, single form
Comment 2 Robert Großkopf 2018-05-06 13:25:19 UTC
Could confirm the buggy behavior.
LO crashes while migrating the database with
Version: 6.1.0.0.alpha1+
Build-ID: 4ed3137022efa6128ad146e4b4dfae13548431dc
CPU-Threads: 4; BS: Linux 4.4; UI-Render: Standard; VCL: kde4; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2018-05-04_01:13:51
Gebietsschema: de-DE (de_DE.UTF-8); Calc: group

Don't know why Binary isn't imported as BLOB - Binary. Nobody will use LONGVARCHAR in such a field for HSQLDB.
Comment 3 Drew Jensen 2018-05-06 18:45:04 UTC
Created attachment 141938 [details]
Test file using binary(fix)[BINARY]

Data type of Binary(fix)[BINARY] will also cause Libo to become non responsive.

This test file includes one record with data added via an image control.

Output at stdout: (soffice:14582): GLib-CRITICAL **: 14:37:18.825: g_file_test: assertion 'filename != NULL' failed
malloc(): smallbin double linked list corrupted

If you open the file in hsql mode and delete the record then restart migration it runs to completion and the fb data type is set as: Text(fix)[CHAR]

Test with build
Version: 6.1.0.0.alpha1+
Build ID: f1579d3d6c5f5f3a651825e035b93bee7a4f43c6
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk2; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2018-05-03_10:04:51
Locale: en-US (en_US.UTF-8); Calc: group
Comment 4 Julien Nabet 2018-05-16 20:07:43 UTC
Created attachment 142136 [details]
valgrind trace

On pc Debian x86-64 with master sources updated today, I could reproduce this.

I also reproduced the pb when indicating Yes for the migration when opening form first time.
I retrieved Valgrind trace.
Comment 5 Julien Nabet 2018-05-16 20:50:05 UTC
Created attachment 142137 [details]
bt with debug symbols
Comment 6 Julien Nabet 2018-05-17 16:35:02 UTC
Created attachment 142167 [details]
bt about length of varbinary on hsqldb

Drew, what's the size of the image included in the table?

In hsqldb, VARBINARY is 8000 bytes max (see https://www.tutorialspoint.com/hsqldb/hsqldb_data_types.htm) and https://opengrok.libreoffice.org/xref/core/dbaccess/source/filter/hsqldb/createparser.cxx#148 shows that indeed, we retain 8000 bytes.

But it seems there are several varbinary types, images may be until 2,147,483,647 bytes! (see the same link)

In gdb, breaking in /home/julien/lo/libreoffice/connectivity/source/drivers/firebird/PreparedStatement.cxx:844 gives:
Thread 1 "soffice.bin" hit Breakpoint 2, connectivity::firebird::OPreparedStatement::setBytes (this=0x5555576bdcf0, nParameterIndex=2, 
    xBytes=uno::Sequence of length 20897 = {...}) at /home/julien/lo/libreoffice/connectivity/source/drivers/firebird/PreparedStatement.cxx:839
844	            Sequence<sal_Int8> xBytesCopy(xBytes);
(gdb) p xBytes
$1 = uno::Sequence of length 20897 = {-119 '\211', 80 'P', 78 'N', 71 'G', 13 '\r', 10 '\n', 26 '\032', 10 '\n', 0 '\000', 0 '\000', 0 '\000', 13 '\r', 73 'I', 72 'H', 
  68 'D', 82 'R', 0 '\000', 0 '\000', 3 '\003', 24 '\030', 0 '\000', 0 '\000', 0 '\000', -1 '\377', 8 '\b', 6 '\006', 0 '\000', 0 '\000', 0 '\000', 114 'r', -44 '\324', 
  112 'p', -61 '\303', 0 '\000', 0 '\000', 0 '\000', 4 '\004', 115 's', 66 'B', 73 'I', 84 'T', 8 '\b', 8 '\b', 8 '\b', 8 '\b', 124 '|', 8 '\b', 100 'd', -120 '\210', 
  0 '\000', 0 '\000', 0 '\000', 25 '\031', 116 't', 69 'E', 88 'X', 116 't', 83 'S', 111 'o', 102 'f', 116 't', 119 'w', 97 'a', 114 'r', 101 'e', 0 '\000', 103 'g', 110 'n', 
  111 'o', 109 'm', 101 'e', 45 '-', 115 's', 99 'c', 114 'r', 101 'e', 101 'e', 110 'n', 115 's', 104 'h', 111 'o', 116 't', -17 '\357', 3 '\003', -65 '\277', 62 '>', 
  0 '\000', 0 '\000', 32 ' ', 0 '\000', 73 'I', 68 'D', 65 'A', 84 'T', 120 'x', -100 '\234', -20 '\354', -35 '\335', 123 '{', 88 'X', 85 'U', -25 '\347', -99 '\235', 
  -9 '\367', -1 '\377', -9 '\367', 62 '>', -80 '\260', 1 '\001', -35 '\335', 91 '[', 97 'a', -125 '\203', -70 '\272', 73 'I', 4 '\004', 52 '4', 5 '\005', 91 '[', -95 '\241', 
  26 '\032', 49 '1', -115 '\215', 76 'L', 19 '\023', 73 'I', -46 '\322', 104 'h', 77 'M', 49 '1', 83 'S', 113 'q', -110 '\222', 106 'j', 58 ':', -43 '\325', -103 '\231', 
  -44 '\324', -76 '\264', 83 'S', 109 'm', -89 '\247', 58 ':', -45 '\323', -122 '\206', 103 'g', -6 '\372', -124 '\204', -10 '\366', 55 '7', 33 '!', -41 '\327', -92 '\244', 
  -40 '\330', 105 'i', -75 '\265', -65 '\277', 62 '>', 58 ':', 79 'O', 11 '\v', 51 '3', -119 '\211', -74 '\266', -3 '\375', 73 'I', 103 'g', 18 '\022', 39 '\'', -87 '\251', 
  -104 '\230', 70 'F', -90 '\246', 105 'i', 72 'H', -109 '\223', -112 '\220', -104 '\230', 66 'B', 90 'Z', -79 '\261', 90 'Z', 48 '0', 17 '\021', 26 '\032', 35 '#', 
  -102 '\232', -128 '\200', 81 'Q', -74 '\266', 7 '\a', -74 '\266', 2 '\002', -101 '\233', 125 '}', -8 '\370', -3 '\375', -63 '\301', 65 'A', 78 'N'...}
(gdb) 

so we got here a sequence of 20897 values and since they're bytes, 20897 bytes ; to compare with the 8000 bytes allocated...
Comment 7 Julien Nabet 2018-05-17 16:50:54 UTC
Here's a bandaid patch:
iff --git a/connectivity/source/drivers/firebird/PreparedStatement.cxx b/connectivity/source/drivers/firebird/PreparedStatement.cxx
index 0a43d5c93851..61adf79b5637 100644
--- a/connectivity/source/drivers/firebird/PreparedStatement.cxx
+++ b/connectivity/source/drivers/firebird/PreparedStatement.cxx
@@ -843,6 +843,11 @@ void SAL_CALL OPreparedStatement::setBytes(sal_Int32 nParameterIndex,
                 xBytesCopy.realloc( nMaxSize );
             }
             const short nSize = xBytesCopy.getLength();
+            if (nSize > 8000)
+            {
+                free(pVar->sqldata);
+                pVar->sqldata = static_cast<char *>(calloc(nSize + 2, sizeof(char)));
+            }
             memcpy(pVar->sqldata, &nSize, 2);
             // Actual data
             memcpy(pVar->sqldata + 2, xBytesCopy.getConstArray(), nSize);

I use calloc to be sure not letting garbage in allocated memory.
With this, it doesn't crash but I don't see the image in the form.
BTW, I unzipped the odb file and don't see where the image is.

Lionel/Tamas: any thoughts about this patch?
Comment 8 Julien Nabet 2018-05-17 16:59:21 UTC
I forgot to tell, I was investigating about where the value 8000 came from because it's used here:
285 case SQL_VARYING:
286   pVar->sqldata = static_cast<char *>(malloc(sizeof(char)*pVar->sqllen + 2));
(see https://opengrok.libreoffice.org/xref/core/connectivity/source/drivers/firebird/Util.cxx#286)
gdb indicated that pVar->sqllen = 8000 here.
Comment 9 Drew Jensen 2018-05-17 17:06:48 UTC
I created the test file with huge values for those columns 2 Million bytes. HSQL embedded in Base works just fine with that. 

The images were 20 K and 32 K in size, added by linking an image control in a form to them and using that to insert the image. The forms work as expected with the data and reports work with it also.

The reason for a 20k and a 32K file was that I thought Firebird was going to impose a 32K limit for the fixed field. My plan was to first test the migration function with empty tables (seemed to pass) then with this data that would fit into FB and when this passes a test migration , check for error handling when the data exceeds the FB imposed limits.

As for the 8000 byte limit, IIRC, that is for if you want to index that column or if you are using a character set that supports collating.
Comment 10 Drew Jensen 2018-05-26 12:29:58 UTC
some more information.

In the current code the migration assistant ALWAYS, when it finds a varbinary, column in the import file, creates the firebird equivalent field with a size of 8000 bytes (even though firebird supports up to 32k in size for this type of field and I'll open a separate issue for that). // see earlier comments

The test file attached to this issue includes a column with 20k of data.

Running tests with 2K of data in the column did not trigger the hang, running a test with 9k data in the column did.

The Base HSQLdb UI implementation doesn't allow the user to explicitly set the size of a varbinary field. The field size grows to accommodate the largest data value put into it, up to 2Meg (or is it 2Gig?) in size. 

Changing the summary on this issue to just deal with what happens when the migration assistant tries to put 20k into a 8k sack ;)
Comment 11 Julien Nabet 2018-05-26 16:04:32 UTC
For information, I submitted for review on gerrit the patch quoted in comment 7, see https://gerrit.libreoffice.org/#/c/54863/
Comment 12 Drew Jensen 2018-05-26 16:19:48 UTC
Excellent

So there is two new problems here:
First in firbird that column could be up to 32K in size ( and max data would be 32k - 2 bytes IIRC)
Second - with the current migration assistant it creates a column type that is unrecognized and unusable by any current Base data control. The user can connect to a text control or an image control, either way the data is either lost when you try to insert it, mangled or irretrievable.

However, if you do connect an image control to one of these migration assistant created column and open an image less then 8000 bytes in size it appears to write something to the table. If you try to export that data, to Writer for example, what you get is that value x0 and nothing else.
Comment 13 Commit Notification 2018-05-28 18:30:26 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=54ddc4ff4c2ff7e8b2c502d6b475cfdc9b8e3cec

tdf#117446: FB mig, (VAR)BINARY, fix memory management

It will be available in 6.2.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 14 Commit Notification 2018-05-29 05:02:26 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "libreoffice-6-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=570279baa7df076413cc6b4f9220a07248db0c1b&h=libreoffice-6-1

tdf#117446: FB mig, (VAR)BINARY, fix memory management

It will be available in 6.1.0.1.

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

Affected users are encouraged to test the fix and report feedback.
Comment 15 Xisco Faulí 2018-06-06 18:18:30 UTC
LibreOffice doesn't hang in

Version: 6.1.0.0.beta1+
Build ID: 8b96445766efe237eb47608ade6c147673466e2e
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group threaded

@Julien, @Drew Jensen, Since this issue is about Libreoffice hanging, should it be closed as RESOLVED FIXED ?
Comment 16 Drew Jensen 2018-06-06 18:24:38 UTC
yes, the attached file and two others which did previously no longer do.

test with
Version: 6.1.0.0.beta1+
Build ID: b57819201ff89bbde14b4df8c02a0eb4bea2c2bb
Branch:libreoffice-6-1, Time: 2018-06-04_04:41:46
Comment 17 MM 2018-06-09 12:53:10 UTC
With Version: 6.2.0.0.alpha0+
Build ID: c4c56de1b0e62ec866b519b2b24c5e805f0a86d3
CPU threads: 2; OS: Linux 4.4; UI render: default; VCL: gtk2; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2018-06-09_00:23:35
Locale: en-US (en_US.UTF-8); Calc: threaded

it doesn't crash anymore, but the patch has some side effects.
The picture from attachment 141910 [details] is gone now if you migrate. And if you add a new image (any), it will also disappear when closing / reopening form 'tbl_varbinary'.
Comment 18 Julien Nabet 2018-06-09 16:28:50 UTC
(In reply to MM from comment #17)
> ...
> it doesn't crash anymore, but the patch has some side effects.
> The picture from attachment 141910 [details] is gone now if you migrate. And
> if you add a new image (any), it will also disappear when closing /
> reopening form 'tbl_varbinary'.

Difficult to tell if it's a side effect since before the patch, there was a hang (or I missed something). I had noticed the disappearing of image too but I don't know why. At least the patch fixed the hang.
Comment 19 Drew Jensen 2018-06-09 16:51:53 UTC
(In reply to MM from comment #17)
> With Version: 6.2.0.0.alpha0+
> Build ID: c4c56de1b0e62ec866b519b2b24c5e805f0a86d3
> CPU threads: 2; OS: Linux 4.4; UI render: default; VCL: gtk2; 
> TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time:
> 2018-06-09_00:23:35
> Locale: en-US (en_US.UTF-8); Calc: threaded
> 
> it doesn't crash anymore, but the patch has some side effects.
> The picture from attachment 141910 [details] is gone now if you migrate. And
> if you add a new image (any), it will also disappear when closing /
> reopening form 'tbl_varbinary'.

Right, but this issue was just about the application becoming non-responsive, that is fixed. 

The image disappearing is a separate issue, but even that is ancillary. 

There is a question as to how valid the test file is. Not that you can't store an image that way in the hsqldb version of Base, the file itself shows you can, but whether it is how anyone is using it. (it would be less than optimal performance wise).

These are two datatypes not directly found in the Firebird engine, and the native firedbird datatype used in the new Firebird schema doesn't currently support moving data in and out via an image control. But again that is a separate issue, and someone should open it. ;-)