This is an automated email from the ASF dual-hosted git repository.
leginee pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/openoffice.git
The following commit(s) were added to refs/heads/trunk by this push:
new 210da5364e Bz126990 file corruption at shutdown trunk (#493)
210da5364e is described below
commit 210da5364ee5715a5b2c0345cd5580a3bfc88049
Author: Peter <[email protected]>
AuthorDate: Mon Aug 31 21:36:27 2026 +0000
Bz126990 file corruption at shutdown trunk (#493)
Test confirms fix:
https://lists.apache.org/thread/m2plf5f2rtkmy7rdvnlbt5vjxk263m6d
merging the fix
* ucb/ucpfile: flush file data to the medium before reporting a write
successful
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 0387c05dadfcf647c52ce960f2e148e65ac63c00)
* 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)
* update license
---------
Co-authored-by: Claude Opus 5 <[email protected]>
---
main/sfx2/source/doc/docfile.cxx | 110 +++++++++++++++++++++++++--
main/sfx2/source/doc/readme.md | 151 +++++++++++++++++++++++++++++++++++++
main/ucb/source/ucp/file/shell.cxx | 17 +++++
3 files changed, 272 insertions(+), 6 deletions(-)
diff --git a/main/sfx2/source/doc/docfile.cxx b/main/sfx2/source/doc/docfile.cxx
index 21ffbddb0f..81f0cfeb0d 100644
--- a/main/sfx2/source/doc/docfile.cxx
+++ b/main/sfx2/source/doc/docfile.cxx
@@ -1788,6 +1788,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,
@@ -1851,12 +1934,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..908c2272fb
--- /dev/null
+++ b/main/sfx2/source/doc/readme.md
@@ -0,0 +1,151 @@
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+# 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
+```
diff --git a/main/ucb/source/ucp/file/shell.cxx
b/main/ucb/source/ucp/file/shell.cxx
index 0297f02eca..8acae4dd9f 100644
--- a/main/ucb/source/ucp/file/shell.cxx
+++ b/main/ucb/source/ucp/file/shell.cxx
@@ -1905,6 +1905,23 @@ shell::write( sal_Int32 CommandId,
}
} while( nReadBytes == nRequestedBytes );
+ // Force the data onto the physical medium before success is reported.
+ // Closing alone only flushes osl's own buffer into the OS page cache; the
+ // file system journals the new file size but not the data, so a crash or
+ // power loss between here and the next writeback leaves a file of the
+ // right length containing nothing but zeros (i126990).
+ if( bSuccess )
+ {
+ err = aFile.sync();
+ if( err != osl::FileBase::E_None )
+ {
+ installError( CommandId,
+ TASKHANDLING_FILEIOERROR_FOR_WRITE,
+ err );
+ bSuccess = sal_False;
+ }
+ }
+
err = aFile.close();
if( err != osl::FileBase::E_None )
{
