If LO has a file open, and then the permissions of the directory are changed to prevent writing, then the lock file will not be removed when LO closes. If you then try to write overtop of the file, LO will crash (but only if the lock file exists). Granted, this is somewhat of a contrived example, but it is a reproducible way of making LO crash. Steps to reproduce. 1.) make a writeable directory (/tmp/crash) 2.) have LO save a document in that directory (/tmp/crash/junk.docx) 3.) while LO still has the document open (so that /tmp/crash/.~lock.junk.docx# exists), chmod -w /tmp/crash 4.) restart LO and start a new document. 5.) save as /tmp/crash/junk.docx - crash after agreeing to overwrite If the lock file is removed, LO will not crash, but just say no permissions. Prior to 6.1 LO was able to modify the file (since the file itself has +w permissions, it can be changed, just not deleted). commit 5259ab8104cfba60c40748ed0cd59d93df038c5b Author: Miklos Vajna on Mon Jan 8 15:53:58 2018 +0100 sfx2 store: create temp files next to local files Doesn't look like a regression, just exposed a crash-able code path.
Reproduced with: Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: f42363c51672a5b3685b0b9b11e932680530dce3 CPU threads: 8; OS: Linux 6.5; UI render: default; VCL: gtk3 Locale: en-AU (en_AU.UTF-8); UI: en-US Calc: CL threaded Crash report with signature "sax_fastparser::FastSerializerHelper::endDocument()" in 7.6: https://crashreport.libreoffice.org/stats/crash_details/ee94c0cd-2191-42df-a57f-995391ae9498
IIRC we already do some check to see if this file is a symlink, etc. Perhaps that could be extended to also check for read-onlyness? It's annoying we don't have a Linux equivalent of Windows' ReplaceFileW().
Created attachment 193113 [details] bt with console logs On pc Debian x86-64 with master sources updated today, I could reproduce this. I attached bt with console logs.
I gave a try with https://gerrit.libreoffice.org/c/core/+/164808
Julien Nabet committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/2887ffbf240aa70330cb50bf810170cf9c896405 tdf#160192: fix crash when trying to overwrite file in RO dir+lock file It will be available in 24.8.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.
Justin L/Miklos: I read about Justin's comment on gerrit about doc/xls/ppt case which leads to an infinite loop. With master sources including the patch, I gave a try with a doc file and reproduced this. It looped here: 1081 while( (rStrm.Tell() & 3) != 0 ) 1082 rStrm.WriteUChar( 0 ); part of bt: #0 0x00007fe4017e0b73 in SvStream::WriteUChar(unsigned char) (this=0x562c2650f2a0, v=0 '\000') at /home/julien/lo/libreoffice/tools/source/stream/stream.cxx:998 #1 0x00007fe3fe7f2b36 in SfxOleSection::SaveProperty(SvStream&, SfxOlePropertyBase&, unsigned long&) (this=0x562c266abd10, rStrm=..., rProp=..., rnPropPosPos=@0x7ffc3d4420c0: 18446744073709551615) at /home/julien/lo/libreoffice/sfx2/source/doc/oleprops.cxx:1082 #2 0x00007fe3fe7f291f in SfxOleSection::ImplSave(SvStream&) (this=0x562c266abd10, rStrm=...) at /home/julien/lo/libreoffice/sfx2/source/doc/oleprops.cxx:1016 #3 0x00007fe3fe7eee97 in SfxOleObjectBase::Save(SvStream&) (this=0x562c266abd10, rStrm=...) at /home/julien/lo/libreoffice/sfx2/source/doc/oleprops.cxx:349 #4 0x00007fe3fe7eef96 in SfxOleObjectBase::SaveObject(SvStream&, SfxOleObjectBase&) (this=0x7ffc3d442698, rStrm=..., rObj=...) at /home/julien/lo/libreoffice/sfx2/source/doc/oleprops.cxx:361 #5 0x00007fe3fe7f3bdb in SfxOlePropertySet::ImplSave(SvStream&) (this=0x7ffc3d442698, rStrm=...) at /home/julien/lo/libreoffice/sfx2/source/doc/oleprops.cxx:1213 #6 0x00007fe3fe7eee97 in SfxOleObjectBase::Save(SvStream&) (this=0x7ffc3d442698, rStrm=...) at /home/julien/lo/libreoffice/sfx2/source/doc/oleprops.cxx:349 #7 0x00007fe3fe7f32b9 in SfxOlePropertySet::SavePropertySet(SotStorage*, rtl::OUString const&) (this=0x7ffc3d442698, pStrg=0x562c22fe48b0, rStrmName="\005SummaryInformation") at /home/julien/lo/libreoffice/sfx2/source/doc/oleprops.cxx:1114 #8 0x00007fe3fe70cbdb in sfx2::SaveOlePropertySet(com::sun::star::uno::Reference<com::sun::star::document::XDocumentProperties> const&, SotStorage*, com::sun::star::uno::Sequence<signed char> const*, com::sun::star::uno::Sequence<signed char> const*, com::sun::star::uno::Sequence<signed char> const*) (i_xDocProps=uno::Reference to ((anonymous namespace)::SfxDocumentMetaData *) 0x562c1e7b6cc8, i_pStorage=0x562c22fe48b0, i_pThumb=0x0, i_pGuid=0x0, i_pHyperlinks=0x0) at /home/julien/lo/libreoffice/sfx2/source/doc/docinf.cxx:243 #9 0x00007fe2efc67cba in WW8Export::PrepareStorage() (this=0x7ffc3d443068) at /home/julien/lo/libreoffice/sw/source/filter/ww8/wrtww8.cxx:3794 #10 0x00007fe2efc659b2 in WW8Export::ExportDocument_Impl() (this=0x7ffc3d443068) at /home/julien/lo/libreoffice/sw/source/filter/ww8/wrtww8.cxx:3551 First I tried this: diff --git a/sfx2/source/doc/oleprops.cxx b/sfx2/source/doc/oleprops.cxx index 891110c43780..d37838c9c562 100644 --- a/sfx2/source/doc/oleprops.cxx +++ b/sfx2/source/doc/oleprops.cxx @@ -1110,7 +1110,7 @@ ErrCode const & SfxOlePropertySet::SavePropertySet( SotStorage* pStrg, const OUS if( pStrg ) { rtl::Reference<SotStorageStream> xStrm = pStrg->OpenSotStream( rStrmName, StreamMode::TRUNC | StreamMode::STD_WRITE ); - if( xStrm.is() ) + if( xStrm.is() && xStrm->GetErrorCode() == ERRCODE_NONE) Save( *xStrm ); else SetError( ERRCODE_IO_ACCESSDENIED ); no loop then but had an std::abort. Then I thought a write access pb should be dealt at the earliest moment, so PrepareStorage could be a good candidate to detect a pb. Here is a second try: diff --git a/sw/source/filter/ww8/wrtww8.cxx b/sw/source/filter/ww8/wrtww8.cxx index 23bc15c8f941..88bc00e85a92 100644 --- a/sw/source/filter/ww8/wrtww8.cxx +++ b/sw/source/filter/ww8/wrtww8.cxx @@ -3548,7 +3548,14 @@ bool SwWW8Writer::InitStd97CodecUpdateMedium( ::msfilter::MSCodec_Std97& rCodec ErrCode WW8Export::ExportDocument_Impl() { - PrepareStorage(); + try + { + PrepareStorage(); + } + catch(uno::RuntimeException&) + { + return ERRCODE_IO_CANTWRITE; + } m_pFib.reset(new WW8Fib(8, m_bDot)); @@ -3767,6 +3774,11 @@ void WW8Export::PrepareStorage() aGName, SotClipboardFormatId::NONE, "Microsoft Word-Document"); rtl::Reference<SotStorageStream> xStor(GetWriter().GetStorage().OpenSotStream(sCompObj)); xStor->WriteBytes(pData, sizeof(pData)); + if (xStor->GetError() != ERRCODE_NONE) + { + SAL_WARN( "sw.ww8", "PrepareStorage failed" ); + throw uno::RuntimeException("PrepareStorage failed"); + } SwDocShell* pDocShell = m_rDoc.GetDocShell (); OSL_ENSURE(pDocShell, "no SwDocShell"); No crash, no hang and the right error message. Now I hesitate between using exception management (which seems a bit quick and dirty) or adding a return value to PrepareStorage but it would be a slightly bigger patch: diff --git a/sw/source/filter/ww8/wrtww8.cxx b/sw/source/filter/ww8/wrtww8.cxx index 23bc15c8f941..de6a9edc0873 100644 --- a/sw/source/filter/ww8/wrtww8.cxx +++ b/sw/source/filter/ww8/wrtww8.cxx @@ -3548,7 +3548,10 @@ bool SwWW8Writer::InitStd97CodecUpdateMedium( ::msfilter::MSCodec_Std97& rCodec ErrCode WW8Export::ExportDocument_Impl() { - PrepareStorage(); + ErrCode err = PrepareStorage(); + + if (err != ERRCODE_NONE) + return err; m_pFib.reset(new WW8Fib(8, m_bDot)); @@ -3642,7 +3645,6 @@ ErrCode WW8Export::ExportDocument_Impl() StoreDoc1(); - ErrCode err = ERRCODE_NONE; if ( bEncrypt ) { SvStream *pStrmTemp, *pTableStrmTemp, *pDataStrmTemp; @@ -3736,7 +3738,7 @@ ErrCode WW8Export::ExportDocument_Impl() return err; } -void WW8Export::PrepareStorage() +ErrCode WW8Export::PrepareStorage() { static const sal_uInt8 pData[] = { @@ -3767,11 +3769,17 @@ void WW8Export::PrepareStorage() aGName, SotClipboardFormatId::NONE, "Microsoft Word-Document"); rtl::Reference<SotStorageStream> xStor(GetWriter().GetStorage().OpenSotStream(sCompObj)); xStor->WriteBytes(pData, sizeof(pData)); + const ErrCode& err = xStor->GetError(); + if (err != ERRCODE_NONE) + { + SAL_WARN( "sw.ww8", "PrepareStorage failed" ); + return err; + } SwDocShell* pDocShell = m_rDoc.GetDocShell (); OSL_ENSURE(pDocShell, "no SwDocShell"); - if (!pDocShell) return; + if (!pDocShell) return ERRCODE_NONE; uno::Reference<document::XDocumentPropertiesSupplier> xDPS( pDocShell->GetModel(), uno::UNO_QUERY_THROW); @@ -3780,7 +3788,7 @@ void WW8Export::PrepareStorage() OSL_ENSURE(xDocProps.is(), "DocumentProperties is null"); if (!xDocProps.is()) - return; + return ERRCODE_NONE; if (officecfg::Office::Common::Filter::Microsoft::Export::EnableWordPreview::get()) { @@ -3792,6 +3800,7 @@ void WW8Export::PrepareStorage() } else sfx2::SaveOlePropertySet( xDocProps, &GetWriter().GetStorage() ); + return ERRCODE_NONE; } ErrCodeMsg SwWW8Writer::WriteStorage() diff --git a/sw/source/filter/ww8/wrtww8.hxx b/sw/source/filter/ww8/wrtww8.hxx index e69d35e07c7a..ac630d9332b7 100644 --- a/sw/source/filter/ww8/wrtww8.hxx +++ b/sw/source/filter/ww8/wrtww8.hxx @@ -1041,7 +1041,7 @@ private: /// Format-dependent part of the actual export. virtual ErrCode ExportDocument_Impl() override; - void PrepareStorage(); + ErrCode PrepareStorage(); void WriteFkpPlcUsw(); void WriteMainText(); void StoreDoc1(); Here too no crash, no hang and the right error message and it lets the possibility to better deal with cases like "if (!pDocShell)" or "if (!xDocProps.is())" What do you think?
Justin Luth committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/c615fba780da5ac3073fe5a00848824f59ee376a tdf#160192: avoid hang when trying to overwrite doc/lockfile in RO dir It will be available in 24.8.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.
Justin Luth committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/f56b2dcb5f8b264366a7f7945f3d5ad83aaa7ea8 tdf#160192: avoid crash when trying to overwrite doc/lockfile in RO dir It will be available in 24.8.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.
I unassign myself since you better deal than about all this stuff. Anyway, I updated my local build to f56b2dcb5f8b264366a7f7945f3d5ad83aaa7ea8 and I kept on the test. Each time, I used the file name "test" (test.xls, test.ppt, test.ppt) 1) with xls file when trying to save the second time when /tmp/crash has no write permission, I got: Error saving the document "Untitled 1": Read Error. The file could not be opened. 2) with ppt file when trying to save the second time when /tmp/crash has no write permission, I got: Error saving the document "Untitled 1": Write Error. The file could not be written. 3) with doc file when trying to save the second time when /tmp/crash has no write permission, I got: Error saving the document "Untitled 1": Object not accessible. The object cannot be accessed due to insufficient user rights. => file name : document is "Untitled 1" in the 3 cases (xls, ppt, doc) whereas I expected the right file name message : "doc" : insufficient user rights => ok "xls" : read error => ko, should be modified. "ppt" : write pb => ok but perhaps it should be better to have "insufficient user rights" too
Justin Luth committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/5441ed518d36ac12a6bd0c517e2cfc24aff94d50 tdf#40244 tdf#160192: create tempFile elsewhere if LogicBase is RO It will be available in 24.8.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.
(In reply to Julien Nabet from comment #9) > "doc" : insufficient user rights => ok > "xls" : read error => ko, should be modified. > "ppt" : write pb => ok but perhaps it should be better to have "insufficient > user rights" too comment 10's patch basically overrides all of the previous patches since it provides an alternate location to create the tempfile, thus avoiding the unavailable file crashes. In my testing, even though the folder was set to read-only, I had left the file and lock-file as read-write. With comment 10's patch, I was able to save-as overtop, and then continue to save changes afterwards. So I think all identified problem areas have been solved.
Julien Nabet committed a patch related to this issue. It has been pushed to "libreoffice-24-2": https://git.libreoffice.org/core/commit/fe9beede27ce613f01fe51d0c7cc894878d4b31e tdf#160192: fix crash when trying to overwrite file in RO dir+lock file It will be available in 24.2.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.