Fix: one OLE2FRAME with no payload throws away the whole document - #1233
Fix: one OLE2FRAME with no payload throws away the whole document#1233redbluevn wants to merge 2 commits into
Conversation
CadOle2FrameTemplate recovers the frame's four corners by parsing the OLE2 payload, and built a StreamIO over CadObject.BinaryData without checking there was one. AutoCAD's own DXF export writes an OLE2FRAME's geometry as XDATA under OLEBEGIN and emits NO binary chunk at all, so BinaryData is null and the constructor throws ArgumentNullException. That exception comes out of CadDocumentBuilder.BuildDocument, so it is not one frame that is lost - it is the whole file. A client drawing of 32,571 entities holding twenty OLE frames could not be read back from AutoCAD's own DXF export of it at all. Found by asking a question the round-trip census cannot: not "did we keep what we read" but "did we read what is there". Comparing our reading of a DWG against our reading of AutoCAD's DXF export of the same drawing crashed on the second half before it could compare anything. The guard also covers a payload too short for the block that follows - two bytes and four points of three doubles - rather than only the null case, and notifies rather than失败 silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| /// ArgumentNullException came out of BuildDocument, so one frame lost the entire document: a | ||
| /// drawing of 32,571 entities holding twenty of them could not be read back from AutoCAD's DXF. | ||
| /// </summary> | ||
| public class Ole2FrameWithoutPayloadTests |
There was a problem hiding this comment.
Tests not needed, you can remove it.
| // | ||
| //Two bytes of header and four points of three doubles is what the block below consumes. | ||
| const int required = 2 + (4 * 3 * 8); | ||
| if (this.CadObject.BinaryData == null || this.CadObject.BinaryData.Length < required) |
There was a problem hiding this comment.
This can be simplified, the Ole2Frame implementation is not complete right now, instead add an 'else' with a return.
There was a problem hiding this comment.
Simplified in 28cefb4 — the length check and the notification are gone, and the guard is now an else that returns.
One detail worth flagging, since it is not quite a bare else: it reads else if (this.CadObject.BinaryData == null). The two readers fill BinaryData by different routes — the DXF reader collects the 310 chunks into Chunks (DxfSectionReaderBase.cs:1697), while the DWG reader assigns BinaryData directly and never touches Chunks (DwgObjectReader.cs, ReadBytes(dataLength)). A plain else { return; } would therefore skip corner parsing for every OLE2FRAME read from DWG. Happy to shape it differently if you prefer.
Checked the fix still holds without the test: a DXF whose OLE2FRAME carries no 310 chunk no longer throws ArgumentNullException out of BuildDocument. Suite 2313/2332 on this branch, the same 17 failures as upstream master.
Asked for in review of DomCR#1233: drop the test, and replace the guard with an else that returns. The else is on the chunk branch, not a bare else, because the two readers fill BinaryData by different routes: the DXF reader collects binary chunks into Chunks (DxfSectionReaderBase.cs), while the DWG reader assigns BinaryData directly (DwgObjectReader.cs, ReadBytes(dataLength)) and leaves Chunks empty. A plain "else return" would therefore skip corner parsing for every OLE2FRAME read from DWG. The length check and the notification are dropped with the rest; only the null case is measured, and that is the one AutoCAD's own DXF export produces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ceo4HJNunc2XfA9PnLCpQK
The problem
CadOle2FrameTemplate.buildrecovers the frame's four corners by parsing the OLE2 payload, and builds aStreamIOoverCadObject.BinaryDatawithout checking there is one:AutoCAD's own DXF export writes an OLE2FRAME's geometry as XDATA —
1001 ACAD/1000 OLEBEGIN/1070+1040pairs — and emits no binary chunk at all. SoChunksis empty,BinaryDatastays null, andnew MemoryStream(null)throwsArgumentNullException.That exception comes out of
CadDocumentBuilder.BuildDocument, so it is not one frame that is lost — it is the whole file. A drawing of 32,571 entities holding twenty OLE frames could not be read back from AutoCAD's own DXF export of it at all:The fix
Return early when there is no payload, or one too short for the block that follows (two bytes plus four points of three doubles), and notify rather than fail silently. The corners keep their defaults, which is all that can honestly be said when the data they come from is absent.
How it was found
By asking a question the round-trip census cannot: not "did we keep what we read" but "did we read what is there". Comparing a reading of a DWG against a reading of AutoCAD's DXF export of the same drawing crashed on the second half before it could compare anything.
Tests
Ole2FrameWithoutPayloadTestswrites a real document, splices in anOLE2FRAMEshaped the way AutoCAD writes one — subclass, flags, anOLElabel, and no310anywhere — and asserts the rest of the document survives.Worth noting how the first attempt at this test was useless: constructing an
Ole2Framein code and round-tripping it passed against the unfixed code, because the writer never emits an entity in that state. Only a fixture that reproduces AutoCAD's actual shape goes red. It does, with exactly the exception above.dotnet teston this branch: 2314 passed / 17 failed, againstmasterat 592d70a on this machine at 2313 / 17 — the same pre-existing failures, plus the one test added here.Related
#1223-#1232, DomCR/CSUtilities#23.