On error, SetFilePointer returns INVALID_SET_FILE_POINTER, not 0

[mikekaganski]

Commit 0c73f2c is described to
properly handle failures to map shared memory. It introduced this
check, obviously treating zero returned from SetFilePointer as an
error condition.

But according to [1], the error code is actually
INVALID_SET_FILE_POINTER; and it had been already checked in the
code also touched by the said commit.

This looks to be a problem I see locally testing an embedded database
in LibreOffice. The code creates a new 'fb12_trace.1' file, then this
check gives 0 (obviously, because the file is yet empty), and loops
indefinitely.

The change fixes the infinite loop.

[1] https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-setfilepointer

[hvlad]

It introduced this
check, obviously treating zero returned from SetFilePointer as an
error condition.

The code checks for file size, not for error condition, and it is correct to compare with zero in this case.
Zero means that initializing process still in progress and current process should wait for completion and retry.
If SetFilePointer() returns INVALID_SET_FILE_POINTER there, the result will be the same - close handles and retry.
Note, Firebird never uses shared memory with size zero.
So, while I believe you "fixed" something, I not sure the fix is correct.
Could you provide reproducible example or more info about the issue ?

[mikekaganski]

The code checks for file size, not for error condition, and it is correct to compare with zero in this case.

Thanks for clarification!

Could you provide reproducible example or more info about the issue ?

I have just filed https://bugs.documentfoundation.org/show_bug.cgi?id=144172 with the attached DB. Note that I debugged LibreOffice file with macros to find this place; I have no idea if it's possible to repro the problem without the said environment.

I am willing to help you to create a LibreOffice debugging environment if you want; or if you prefer, to provide you a remote access to my testing system for debugging.

[mikekaganski]

FTR: the database is from this original issue (in Russian).

[hvlad]

I am willing to help you to create a LibreOffice debugging environment if you want;

Let's try this way

[mikekaganski]

Let's try this way

If you can, please join #libreoffice-dev at Libera.Chat. Or you may contact me at telegram (Mike Kaganski). It would likely be easier if I could assist you on-line. If you prefer some other methods, including a video call, please tell.

First of all, you need to set up LO development environment on Windows, as described on https://wiki.documentfoundation.org/Development/BuildingOnWindows. It requires VS2019, and cygwin.

When you reach Configure and build the code stage, note that you need to use --enable-dbgutil flag for autogen.sh, because it needs a complete build, and if you miss it at first, you will likely loose a couple of hours building LO first, then cleaning and building second time.

Please ask whenever you see something unclear.

And BIG THANK YOU for looking at this! (And to all your team for your great DB!)

[mikekaganski]

It looks like I was confused, and it was #4376 that caused the problem: I had several instances of LibreOffice Base running at the same time (in fact, one was hung - partly crashed, only present in process list, without UI).

And here is the scenario where we need #4376: LibreOffice uses its own temp directory (named e.g. %TEMP%\lu27808b4atam.tmp on Windows), which it configures for LOCK directory; it's different for each instance of LibreOffice, and also different for e.g. isql. Note that LibreOffice uses the FB embed DLL to open its embedded databases, which are wholly managed by LibreOffice itself, extracted from LibreOffice ODB package files into temporary directories, and are guaranteed to never be accessed by several instances using different LOCK directories.

So the repro scenario is just to run isql connecting to a local DB file; and open LibreOffice with an embedded FB database and try to open its tables.

[mikekaganski]

It looks like we may have a workaround for now re-defining COMMON_FILE_PREFIX in src/common/file_params.h ...

[hvlad]

I'm trying to build LO, it is nor fast nor easy, unfortunately.

As for COMMON_FILE_PREFIX - it was introduced in fb4 while LO uses fb3, isn't is ?
Before going further with it, could you explain -what is LO instance ?
Is it separate LO process ?
Or is it instance per open document ?
Is it possible that same LO process load more than one FB embedded engine (from different tmp folders) at the same time ?

[mikekaganski]

I'm trying to build LO, it is nor fast nor easy, unfortunately.

As said, I'm ready to help you in any way - just ping me if you need.

As for COMMON_FILE_PREFIX - it was introduced in fb4 while LO uses fb3, isn't is ?

Yes; and I realized that after reading the code that we use. But as well, we may just use process id instead of session id in the ConfigStorage::ConfigStorage creating filename. That would be even better, fitting better into our per-process temp dir policy.

Before going further with it, could you explain -what is LO instance ?
Is it separate LO process ?
Or is it instance per open document ?

It is a process, not per-document.

Is it possible that same LO process load more than one FB embedded engine (from different tmp folders) at the same time ?

No. It uses a DLL that is bundled with the program; it may open different DB files (FDB/FBK) from different directories in the same process though.

[aafemt]

The whole idea for an office package to use client-server DBMS as backend is strange and failures are unavoidable.

[mikekaganski]

Thanks for this feedback - but it is completely useless.

LibreOffice (and OOo before it) offers Base module, something similar to MS Access, which may be used as a front-end for a DB connection, or it allows also to use an embedded database. HSQLDB was used originally; there was some process of choosing a replacement that would allow to drop a Java dependency; FB was chosen there [1]. (Also other options were considered, like SQLite [2].)

It is indeed not trivial to do everything properly here; and we indeed know that there might be problems in the process. We used to have a good support from FB community (a blog of Marius Popa Adrian is e.g. listed in our The LibreOffice Planet). We hope to continue this good relationship; and the comment stating the obvious ("there will be problems") and wrong ("it should not be done") are not that useful.

[1] https://bugs.documentfoundation.org/show_bug.cgi?id=51781
[2] https://bugs.documentfoundation.org/show_bug.cgi?id=38811

[aafemt]

There is a difference between file-server DBMSs and client-server DBMSs. You decided to replace former with later.

[mikekaganski]

There is a difference between file-server DBMSs and client-server DBMSs. You decided to replace former with later.

FB describes its embedded mode [1]:

Needless to say, this is not for your regular client-server database usage.

However it is implemented internally, the embedded engine is created for the use similar/same to what we need.

[1] https://www.firebirdsql.org/pdfmanual/html/ufb-cs-embedded.html

[aafemt]

LibreOffice uses its own temp directory (named e.g. %TEMP%\lu27808b4atam.tmp on Windows), which it configures for LOCK directory;

Accessing the same database with different lock directories will destroy it.

[aafemt]

FB describes its embedded mode [1]:

You misunderstood the concept. Firebird cannot work the way you need.

[mikekaganski]

LibreOffice uses its own temp directory (named e.g. %TEMP%\lu27808b4atam.tmp on Windows), which it configures for LOCK directory;

Accessing the same database with different lock directories will destroy it.

Please, could you first read the above before posting?

I already wrote:

Note that LibreOffice uses the FB embed DLL to open its embedded databases, which are wholly managed by LibreOffice itself, extracted from LibreOffice ODB package files into temporary directories, and are guaranteed to never be accessed by several instances using different LOCK directories.

You misunderstood the concept. Firebird cannot work the way you need.

Could you please elaborate why exactly? IIUC, people from FB community were helping us in the process, and never told that it can't be done. (But this specific discussion is possibly OT in this specific issue.)

[hvlad]

But as well, we may just use process id instead of session id in the ConfigStorage::ConfigStorage creating filename. That would be even better, fitting better into our per-process temp dir policy.

I tend to agree with you here. It looks safe assuming LO uses SuperServer only.

[asfernandes]

The whole idea for an office package to use client-server DBMS as backend is strange and failures are unavoidable.

Firebird embedded is not client-server!

[hvlad]

BTW, I'm going to fix infinite loop.

[mikekaganski]

I tend to agree with you here. It looks safe assuming LO uses SuperServer only.

Thanks! I have created https://gerrit.libreoffice.org/c/core/+/121457; I wonder if it's possible to upstream the change in a way to e.g. introduce a configuration option, and thus avoid the local patch.

BTW, I'm going to fix infinite loop.

Thank you!

[hvlad]

I have created https://gerrit.libreoffice.org/c/core/+/121457;

consider the case when LO process have SeCreateGlobalPrivilege set, and add PID-suffix at "then" branch of "if" condition.

I wonder if it's possible to upstream the change in a way to e.g. introduce a configuration option, and thus avoid the local patch.

Not in this way, sorry. We need to carefully implement instance isolation and consider a bit more places in code and use cases ;)

[mikekaganski]

I have created https://gerrit.libreoffice.org/c/core/+/121457;

consider the case when LO process have SeCreateGlobalPrivilege set, and add PID-suffix at "then" branch of "if" condition.

Thanks - done! :)

I wonder if it's possible to upstream the change in a way to e.g. introduce a configuration option, and thus avoid the local patch.

Not in this way, sorry. We need to carefully implement instance isolation and consider a bit more places in code and use cases ;)

Fair enough - I am looking forward, at the time when the proper isolation is implemented in FB; thanks for your help!

[mikekaganski]

Sorry, forgot to close it properly.
Thanks again for all the clarifications and fixes!