This is an automated email from the ASF dual-hosted git repository. leginee pushed a commit to branch bz126990-fileCorruptionAtShutdown-trunk in repository https://gitbox.apache.org/repos/asf/openoffice.git
commit 112a4f2155dd401aca0c503d82591430f6b8a26c Author: Peter Kovacs <[email protected]> AuthorDate: Tue Jul 28 08:26:03 2026 +0200 sfx2: replace the document atomically instead of rewriting it in place Issue 126990; see main/sfx2/source/doc/readme.md for the full analysis. Co-Authored-By: Claude Opus 5 <[email protected]> (cherry picked from commit 6a1923dc3753b5aa0d063f41324dde9fe8ec0f33) --- main/sfx2/source/doc/docfile.cxx | 110 ++++++++++++++++++++++++++++++-- main/sfx2/source/doc/readme.md | 132 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 236 insertions(+), 6 deletions(-) diff --git a/main/sfx2/source/doc/docfile.cxx b/main/sfx2/source/doc/docfile.cxx index 09a48fd1da..16e6919215 100644 --- a/main/sfx2/source/doc/docfile.cxx +++ b/main/sfx2/source/doc/docfile.cxx @@ -1790,6 +1790,89 @@ sal_Bool SfxMedium::StorageCommit_Impl() return bResult; } +//------------------------------------------------------------------ +namespace +{ + +// Replace the file at rTarget with the contents of rSourceContent by streaming +// into a sibling temporary file and renaming that over the target. +// +// The alternative - truncating the target and rewriting it in place - destroys +// the user's document at the moment of truncation, so any interruption before +// the rewrite has reached the medium leaves nothing to recover: the file system +// commits the final file size but not the data, and the document reads back as +// a run of zeros (i126990). Renaming keeps the previous document intact until +// a single atomic operation swaps the new one in, so an interruption costs at +// most the most recent save instead of the whole document. +// +// The replacement is deliberately created in the target's own directory: +// osl_moveFile() passes MOVEFILE_COPY_ALLOWED, which silently degrades a +// cross-volume move into a non-atomic copy+delete, and rename() cannot cross a +// mount point at all. Only a sibling can guarantee the swap is atomic. +sal_Bool lcl_ReplaceTargetAtomically( ::ucbhelper::Content& rSourceContent, + const INetURLObject& rTarget ) +{ + ::rtl::OUString aTargetURL( rTarget.GetMainURL( INetURLObject::NO_DECODE ) ); + if ( !::utl::LocalFileHelper::IsLocalFile( aTargetURL ) ) + return sal_False; + + // An in-place rewrite follows a symbolic link and updates the file it points + // at; a rename would replace the link itself. Leave links to the caller's + // fallback instead of silently turning one into a regular file. + ::osl::DirectoryItem aTargetItem; + if ( ::osl::DirectoryItem::get( aTargetURL, aTargetItem ) == ::osl::FileBase::E_None ) + { + ::osl::FileStatus aTargetStatus( osl_FileStatus_Mask_Type ); + if ( aTargetItem.getFileStatus( aTargetStatus ) == ::osl::FileBase::E_None + && aTargetStatus.getFileType() == ::osl::FileStatus::Link ) + return sal_False; + } + + INetURLObject aTargetDir( rTarget ); + if ( !aTargetDir.removeSegment() ) + return sal_False; + + // name the replacement after the target, so that a leftover left behind by a + // crash is recognizable as belonging to this document + ::rtl::OUString aFileName = rTarget.getName( INetURLObject::LAST_SEGMENT, true, INetURLObject::NO_DECODE ); + sal_Int32 nPrefixLen = aFileName.lastIndexOf( '.' ); + String aPrefix = ( nPrefixLen == -1 ) ? aFileName : aFileName.copy( 0, nPrefixLen ); + String aExtension = ( nPrefixLen == -1 ) ? String() : String(aFileName.copy( nPrefixLen )); + String aTargetDirURL( aTargetDir.GetMainURL( INetURLObject::NO_DECODE ) ); + + ::utl::TempFile aReplacement( aPrefix, &aExtension, &aTargetDirURL ); + aReplacement.EnableKillingFile( sal_True ); + + ::rtl::OUString aReplacementURL( aReplacement.GetURL() ); + if ( !aReplacementURL.getLength() ) + return sal_False; + + try + { + Reference< ::com::sun::star::ucb::XCommandEnvironment > xDummyEnv; + ::ucbhelper::Content aReplacementContent; + if ( !::ucbhelper::Content::create( aReplacementURL, xDummyEnv, aReplacementContent ) ) + return sal_False; + + // writeStream() forces the data onto the medium before reporting + // success, so the replacement is durable before it is swapped in + aReplacementContent.writeStream( rSourceContent.openStream(), sal_True ); + } + catch( Exception& ) + { + return sal_False; + } + + if ( ::osl::File::move( aReplacementURL, aTargetURL ) != ::osl::FileBase::E_None ) + return sal_False; + + // the replacement is the document now and must not be removed + aReplacement.EnableKillingFile( sal_False ); + return sal_True; +} + +} + //------------------------------------------------------------------ sal_Bool SfxMedium::TransactedTransferForFS_Impl( const INetURLObject& aSource, const INetURLObject& aDest, @@ -1853,12 +1936,27 @@ sal_Bool SfxMedium::TransactedTransferForFS_Impl( const INetURLObject& aSource, if( pImp->m_aBackupURL.getLength() ) { - Reference< XInputStream > aTempInput = aTempCont.openStream(); - bTransactStarted = sal_True; - aOriginalContent.setPropertyValue( ::rtl::OUString::createFromAscii( "Size" ), - uno::makeAny( (sal_Int64)0 ) ); - aOriginalContent.writeStream( aTempInput, bOverWrite ); - bResult = sal_True; + // swap the new contents in with an atomic rename, so that the + // previous document survives untouched if anything goes wrong + bResult = lcl_ReplaceTargetAtomically( aTempCont, aDest ); + + if ( !bResult ) + { + // The rename was not possible - for instance because another + // process holds the target open, which an in-place rewrite + // tolerates but a rename does not. Rather than fail a save + // that used to succeed, fall back to the previous behaviour; + // this reopens the i126990 window, but only in the cases where + // the safe path cannot be taken at all. + OSL_TRACE( "SfxMedium::TransactedTransferForFS_Impl: atomic replace " + "unavailable, falling back to in-place rewrite" ); + Reference< XInputStream > aTempInput = aTempCont.openStream(); + bTransactStarted = sal_True; + aOriginalContent.setPropertyValue( ::rtl::OUString::createFromAscii( "Size" ), + uno::makeAny( (sal_Int64)0 ) ); + aOriginalContent.writeStream( aTempInput, bOverWrite ); + bResult = sal_True; + } } else { diff --git a/main/sfx2/source/doc/readme.md b/main/sfx2/source/doc/readme.md new file mode 100644 index 0000000000..f256ee4d49 --- /dev/null +++ b/main/sfx2/source/doc/readme.md @@ -0,0 +1,132 @@ +# Document save path — atomic replace (Issue 126990) + +Notes on how `SfxMedium` writes a document over an existing file, and why it is +written the way it is. The short version: **never truncate the user's document +before the replacement exists and is on the disk.** + +## The bug this addresses + +Users reported opening a previously saved document and finding it full of `#` +characters — hundreds of reports over many years, always after a crash, freeze, +forced shutdown or power loss. The files were not damaged ODF; they were a run +of zero bytes of roughly the right length. With no `PK` zip header at offset 0, +type detection falls through to the plain text filter, and `0x00` renders as `#`. + +The zeros were never written by us. Two defects combined to produce them: + +1. `TransactedTransferForFS_Impl()` truncated the target to zero (via the `Size` + property) and then streamed the new contents into it. Despite the name, + nothing was transacted — the document was destroyed at the moment of + truncation, and everything after that point was a window in which a crash + cost the whole file. +2. Nothing forced the data to the medium. `osl_closeFile()` flushes osl's own + buffer and calls `CloseHandle()`; it never calls `FlushFileBuffers()`/`fsync()`. + So the bytes sat in the OS page cache while the save reported success. + +File systems that journal metadata but not data — NTFS, HFS+, XFS, and ext4 with +delayed allocation — commit the new file *size* while the data is still only in +the cache. Crash in that window and the file reads back as N zeros. Because the +save had already reported success, users acted on it and lost the document. + +## What the code does now + +Two changes, and they are **not independent — the flush is a prerequisite for the +rename.** Renaming a replacement whose data is still in the page cache would swap +in a file that the same crash turns into zeros, just under a different name. + +- `ucb/source/ucp/file/shell.cxx`, `shell::write()` — syncs before reporting + success, and reports a sync failure as a write failure rather than a silent + success. +- `docfile.cxx`, `lcl_ReplaceTargetAtomically()` — streams the new contents into a + sibling of the target and renames it over the target. The previous document + stays intact until one atomic operation swaps in the new one, so an interruption + now costs at most the most recent save instead of the whole document. + +Note that configmgr has always written `registrymodifications.xcu` this way. The +document save path was the odd one out. + +## Constraints — do not "simplify" these away + +- **The replacement must be a sibling of the target.** `osl_moveFile()` passes + `MOVEFILE_COPY_ALLOWED`, which silently degrades a cross-volume move into a + non-atomic copy+delete, and POSIX `rename()` cannot cross a mount point at all. + `SfxMedium::CreateTempFile()` produces temporaries in the *system* temp + directory — reusing one of those would quietly destroy the atomicity. +- **The in-place path is kept as a fallback, not dead code.** A rename needs + delete access on the target; an in-place rewrite does not. A process holding + the document open — antivirus, a search indexer — would otherwise turn a save + that used to work into a hard failure. That is a worse regression than the rare + corruption this fixes. +- **Symlinked targets deliberately take the fallback.** An in-place rewrite + follows the link and updates what it points at; a rename would replace the link + itself with a regular file. +- **`bTransactStarted` and `UseBackupToRestore_Impl()` stay live** — they are + reachable through the fallback. + +## Known limitations + +- **ACLs, alternate data streams, creation time.** The renamed file carries the + sibling's security descriptor, not the original's. Where ACLs are inherited + from the containing folder — the common case — the result is identical and + nothing is lost; it only differs for explicit per-file ACEs and ADS. Win32 + `ReplaceFile()` is the primitive that preserves all of it, but it is not exposed + by osl, and adding it would mean a new exported sal API. +- **POSIX rename durability.** `rename()` is atomic, but the rename itself is not + durable without an `fsync` of the containing directory, and osl has no + directory-sync API. On Windows `MOVEFILE_WRITE_THROUGH` covers this. The POSIX + failure mode without it is "last save lost", not "document destroyed". +- **macOS `fsync` is weaker than it looks** — it does not flush the drive write + cache; `fcntl(F_FULLFSYNC)` would be required. +- **Peak disk space on the target volume roughly doubles.** The original and the + replacement now coexist until the rename, where the old code held only the file + being rewritten. A large save onto a nearly full volume can therefore hit + `ENOSPC` where it previously did not; that falls back to the in-place path. + Conversely the disk-full case is now *safer*: the original is no longer + destroyed before it is known that the new data fits. +- **The backup is still not durable.** `DoInternalBackup_Impl()` copies via + `transferContent`, which routes to the file UCP's `copy` rather than + `shell::write`, so it does not get the sync. + +## Verifying a build + +The interesting calls are exported Win32 APIs, so this works on a release build +with no PDBs and no debug-level rebuild. Under `cdb`: + +``` +bu KERNELBASE!MoveFileExW ".echo >>> MoveFileExW; du @rcx; du @rdx; gu; r rax; gc" +bu KERNELBASE!FlushFileBuffers ".echo >>> FlushFileBuffers; gc" +``` + +A healthy overwrite save shows flush(es), then a rename whose source and +destination are **in the same directory**, with `r8=b` +(`REPLACE_EXISTING|COPY_ALLOWED|WRITE_THROUGH`) identifying it as `osl_moveFile`: + +``` +>>> FlushFileBuffers +>>> MoveFileExW + C:\Users\...\Documents\mydoc0.odt + C:\Users\...\Documents\mydoc.odt + rax=1 +``` + +Things that will waste your time otherwise: + +- **Save As to a new file never takes the atomic path**, by design — the helper is + behind `bOverWrite && IsDocument(aDest)` and there is nothing yet to protect. + Only an overwrite exercises it. +- **`MoveFileExW` traffic is normal background noise** — configmgr writes its store + the same way, with the same flags. Always read the paths before concluding + anything; `\user\` is config, not the document. +- **The breakpoints fire on entry**, so a *failed* rename looks identical to a + successful one. Capture `rax` (as above) or the fallback is invisible. Without + it, the fallback's tell is one extra `FlushFileBuffers` immediately after the + document rename. +- **`SetEndOfFile` is not a fallback signal** — it also fires when `shell::write` + truncates the sibling that `TempFile` has already created. + +To exercise the fallback deliberately, hold the target open against delete from +another process while saving; the rename fails and the save must still succeed: + +```powershell +$f = [System.IO.File]::Open($p, 'Open', 'Read', 'ReadWrite') # ReadWrite withholds Delete +```
