You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@openoffice.apache.org by pe...@apache.org on 2026/09/18 21:45:05 UTC
(openoffice) branch AOO41X updated (ae9eb83852 -> 2d2c30bc03)
This is an automated email from the ASF dual-hosted git repository.
leginee pushed a change to branch AOO41X
in repository https://gitbox.apache.org/repos/asf/openoffice.git
from ae9eb83852 Limit paths for text caches for embedded databases
new bb401c7a34 Bz126990 file corruption at shutdown trunk (#493)
new 2d2c30bc03 filter: idxf: port the input-validation hunks missed by the #491 backport
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
main/filter/source/graphicfilter/idxf/dxfentrd.cxx | 30 +++-
main/filter/source/graphicfilter/idxf/dxfentrd.hxx | 2 +-
main/filter/source/graphicfilter/idxf/dxfgrprd.cxx | 23 ++--
main/sfx2/source/doc/docfile.cxx | 110 ++++++++++++++-
main/sfx2/source/doc/readme.md | 151 +++++++++++++++++++++
main/ucb/source/ucp/file/shell.cxx | 17 +++
6 files changed, 313 insertions(+), 20 deletions(-)
create mode 100644 main/sfx2/source/doc/readme.md
(openoffice) 02/02: filter: idxf: port the input-validation hunks missed by the #491 backport
Posted by pe...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
leginee pushed a commit to branch AOO41X
in repository https://gitbox.apache.org/repos/asf/openoffice.git
commit 2d2c30bc038b4c4abb51c1ef7101f28570f4ca00
Author: Peter <pe...@apache.org>
AuthorDate: Fri Sep 18 23:43:42 2026 +0200
filter: idxf: port the input-validation hunks missed by the #491 backport
PR #491 was landed on AOO41X as 3c5a7184f8 ("cherry pick and adaptation
of merge commit c082241a9b"), but three hardening hunks of the series did
not survive the adaptation:
* DXFBoundaryPathData::EvaluateGroup, group 93: the vertex count taken
from the file was used for `new DXFVector[n]` without checking the
reader status or the sign. A negative or unread count reached
operator new[]; a huge one threw std::bad_alloc out of the filter.
Check GetStatus() and n >= 0, and turn bad_alloc into a reader error.
* DXFHatchEntity::EvaluateGroup, group 91: identical problem for the
boundary-path count driving `new DXFBoundaryPathData[n]`.
* DXFGroupReader::ReadI: the hand-rolled do/while accumulated digits
without any overflow or termination check, so it also ran once on a
non-digit before testing. Parse with strtol over the digit run and
reject the group unless the converted span ends exactly where the
scan did.
Both allocation sites are reachable from HATCH entities in an untrusted
DXF, which is what makes these worth carrying over rather than leaving to
the next full sync.
Also fixes the "assoiciative" comment typo on nAssociativityFlag, so the
file matches trunk.
No functional change beyond the above: after this commit the idxf sources
on AOO41X are content-identical to the #491 head (96b0524be2), modulo the
trailing-whitespace cleanup trunk applied to the licence headers.
Co-Authored-By: Claude Opus 5 <no...@anthropic.com>
---
main/filter/source/graphicfilter/idxf/dxfentrd.cxx | 30 +++++++++++++++++++---
main/filter/source/graphicfilter/idxf/dxfentrd.hxx | 2 +-
main/filter/source/graphicfilter/idxf/dxfgrprd.cxx | 23 ++++++++++-------
3 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/main/filter/source/graphicfilter/idxf/dxfentrd.cxx b/main/filter/source/graphicfilter/idxf/dxfentrd.cxx
index 27219f03b7..0a46a63483 100644
--- a/main/filter/source/graphicfilter/idxf/dxfentrd.cxx
+++ b/main/filter/source/graphicfilter/idxf/dxfentrd.cxx
@@ -635,8 +635,19 @@ sal_Bool DXFBoundaryPathData::EvaluateGroup( DXFGroupReader & rDGR )
case 93 :
{
nPointCount = rDGR.GetI();
- if ( nPointCount )
- pP = new DXFVector[ nPointCount ];
+ if ( rDGR.GetStatus() && nPointCount >= 0 )
+ {
+ try
+ {
+ pP = new DXFVector[ nPointCount ];
+ }
+ catch (::std::bad_alloc)
+ {
+ rDGR.SetError();
+ }
+ }
+ else
+ rDGR.SetError();
}
break;
case 72 : nHasBulgeFlag = rDGR.GetI(); break;
@@ -719,8 +730,19 @@ void DXFHatchEntity::EvaluateGroup( DXFGroupReader & rDGR )
{
bIsInBoundaryPathContext = sal_True;
nBoundaryPathCount = rDGR.GetI();
- if ( nBoundaryPathCount )
- pBoundaryPathData = new DXFBoundaryPathData[ nBoundaryPathCount ];
+ if ( rDGR.GetStatus() && nBoundaryPathCount >= 0 )
+ {
+ try
+ {
+ pBoundaryPathData = new DXFBoundaryPathData[ nBoundaryPathCount ];
+ }
+ catch (::std::bad_alloc)
+ {
+ rDGR.SetError();
+ }
+ }
+ else
+ rDGR.SetError();
}
break;
case 75 :
diff --git a/main/filter/source/graphicfilter/idxf/dxfentrd.hxx b/main/filter/source/graphicfilter/idxf/dxfentrd.hxx
index ffe8d6fd51..5684658ccd 100644
--- a/main/filter/source/graphicfilter/idxf/dxfentrd.hxx
+++ b/main/filter/source/graphicfilter/idxf/dxfentrd.hxx
@@ -474,7 +474,7 @@ class DXFHatchEntity : public DXFBasicEntity
DXFVector aElevationPoint;
sal_Int32 nFlags; // 70 (solid fill = 1, pattern fill = 0)
- sal_Int32 nAssociativityFlag; // 71 (assoiciative = 1, non-associative = 0)
+ sal_Int32 nAssociativityFlag; // 71 (associative = 1, non-associative = 0)
sal_Int32 nBoundaryPathCount; // 91
sal_Int32 nHatchStyle; // 75 (odd parity = 0, outmost area = 1, entire area = 2 )
sal_Int32 nHatchPatternType; // 76 (user defined = 0, predefined = 1, custom = 2)
diff --git a/main/filter/source/graphicfilter/idxf/dxfgrprd.cxx b/main/filter/source/graphicfilter/idxf/dxfgrprd.cxx
index 6210b18a06..c841086a82 100644
--- a/main/filter/source/graphicfilter/idxf/dxfgrprd.cxx
+++ b/main/filter/source/graphicfilter/idxf/dxfgrprd.cxx
@@ -299,7 +299,6 @@ void DXFGroupReader::ReadLine(char * ptgt)
long DXFGroupReader::ReadI()
{
char sl[DXF_MAX_STRING_LEN+1],*p;
- long res,nv;
ReadLine(sl);
@@ -312,17 +311,23 @@ long DXFGroupReader::ReadI()
return 0;
}
+ char *start = p;
if (*p=='-') {
- nv=-1;
p++;
}
- else nv=1;
-
- res=0;
- do {
- res=res*10+(long)(*p-'0');
+ while (*p>='0' && *p<='9') {
p++;
- } while (*p>='0' && *p<='9');
+ }
+
+ char prev = *p;
+ *p = '\0';
+ char *end;
+ long res = strtol(start, &end, 10);
+ *p = prev;
+ if (end != p) {
+ bStatus=sal_False;
+ return 0;
+ }
while (*p==0x20) p++;
@@ -342,7 +347,7 @@ long DXFGroupReader::ReadI()
return 0;
}
- return res*nv;
+ return res;
}
(openoffice) 01/02: Bz126990 file corruption at shutdown trunk (#493)
Posted by pe...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
leginee pushed a commit to branch AOO41X
in repository https://gitbox.apache.org/repos/asf/openoffice.git
commit bb401c7a345a49f3109504ac895ae33f95e2e136
Author: Peter <le...@users.noreply.github.com>
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 <no...@anthropic.com>
(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 <no...@anthropic.com>
(cherry picked from commit 6a1923dc3753b5aa0d063f41324dde9fe8ec0f33)
* update license
---------
Co-authored-by: Claude Opus 5 <no...@anthropic.com>
(cherry picked from commit 210da5364ee5715a5b2c0345cd5580a3bfc88049)
---
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 61ffcac45c..0ddfc3adef 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..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 417962d629..7c2d3eb42d 100644
--- a/main/ucb/source/ucp/file/shell.cxx
+++ b/main/ucb/source/ucp/file/shell.cxx
@@ -1911,6 +1911,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 )
{