BinTools.Read_s exception: Buffer position calculations in pystreambuf.h produce incorrect results

[bernhard-42]

Issue

Currently the BinTools.Write_s and BinTools.Read_s combination regularly fails.

So I asked claude code to stress test the current implementation to find a reproducible example.

Environment:

micromamba create -n cp79 python=3.13
micromamba activate cp79
micromamba install -c conda-forge -c cadquery OCP=7.9.3.0
micromamba install ipython

Reproducing example

• test.py:

  from OCP.BinTools import BinTools
  from OCP.BRepPrimAPI import BRepPrimAPI_MakeBox, BRepPrimAPI_MakeSphere
  from OCP.BRepAlgoAPI import BRepAlgoAPI_Fuse
  from OCP.TopoDS import TopoDS_Shape
  import io
  
  box = BRepPrimAPI_MakeBox(
      8.189314010683498, 46.85954928498498, 14.39317846913498
  ).Shape()
  
  sphere = BRepPrimAPI_MakeSphere(18.12601828498498).Shape()
  shape =  BRepAlgoAPI_Fuse(box, sphere).Shape()
  
  BinTools.Write_s(shape, "shape.brep")
  
  with open("shape.brep", "rb") as f:
      data = f.read()
  
  buf = io.BytesIO(data)
  result = TopoDS_Shape()
  
  BinTools.Read_s(result, buf)

• Error

  $ python test.py
  Traceback (most recent call last):
    File "/home/bernhard/test.py", line 22, in <module>
      BinTools.Read_s(result, buf)
      ~~~~~~~~~~~~~~~^^^^^^^^^^^^^
  OCP.Standard.Standard_Failure: EXCEPTION in BinTools_ShapeSet::ReadGeometry(S,OS)
  0x558d8ee274a0 : Standard_Failure: BinTools_SurfaceSet::ReadGeometry: UnExpected BRep_PointRepresentation = -1

Note: It fails on my Mac M1 and on my Linux box:

Analysis

The claude code helped, so take it with a pinch of salt, but I thought it makes sense

This shape

• serializes to exactly 5797 bytes
• causes OCCT to request a backward seek during parsing (seek from position 4096 back to 3064)

The backward seek triggers a bug in pystreambuf's position tracking:

Read sequence:
read(1024) × 4 → position 4096
seek(3064) → OCCT wants to re-read earlier data (NORMAL)
read(1024) × 3 → positions 3064 → 4088 → 5112 → 5797 (EOF)
seek(6140) → BUG! pystreambuf calculates wrong position (343 bytes past EOF)
read(1024) → returns 0 bytes → OCCT fails

Why this specific shape?

• The shape's binary representation requires OCCT to seek backward during parsing
• Simple shapes (like a plain box) read sequentially without seeks → work fine
• Complex shapes that need to reference earlier data trigger backward seeks
• The combination of file size (5797) and seek positions causes pystreambuf's buffer arithmetic to overflow

The bug is in pystreambuf.h line ~399-453 (seekoff_without_calling_python), where buffer position calculations after a backward seek
produce incorrect results.

Note: BinTools.Read_s is reliable

Direction	Reliable?	Reason
Write (C++ → Python)	Yes	Sequential, C++ owns buffer, copies to Python
Read (Python → C++)	No	Pointer aliasing, backward seeks break position tracking

Alternative implementation

A much simpler solution we use in https://github.com/jdegenstein/ocp-addons is

py::bytes serialize_shape(const TopoDS_Shape &shape) {
    std::ostringstream buf;
    BinTools::Write(shape, buf);
    return py::bytes(std::move(buf.str()));
}

TopoDS_Shape deserialize_shape(const py::bytes &buf) {
    std::istringstream stream(buf);
    TopoDS_Shape shape;
    BinTools::Read(shape, stream);
    return shape;
}

py::bytes serialize_location(const TopLoc_Location &location) {
    std::ostringstream buf;
    BinTools_OStream occtStream(buf);
    BinTools_ShapeWriter().WriteLocation(occtStream, location);
    return py::bytes(std::move(buf.str()));
}

TopLoc_Location deserialize_location(const py::bytes &buf) {
    std::istringstream stream(buf);
    BinTools_IStream occtStream(stream);
    // This is not a memory leak and can only be copied due to weird occt impl
    return *BinTools_ShapeReader().ReadLocation(occtStream);
}

Side-by-Side Comparison

Comparing both implementations, claude code came up with:

Aspect	pystreambuf	ocp-addons
Buffer ownership	Shared/complex (Python bytes + C++ pointers)	Clear (C++ owns during operation)
Python calls during I/O	Many (~N/1024 for N bytes)	Zero (only at start/end)
GIL safety	Risky (pointers into Python memory)	Safe (pure C++ during I/O)
Memory pattern	Incremental chunks	Single allocation
Seek support	Complex, error-prone	Native C++ stringstream (reliable)
Error handling	Complex (Python exceptions mid-stream)	Simple (fails at boundaries)
Code complexity	~500 lines	~15 lines
Streaming large files	Theoretically better (incremental)	Requires full memory

• Advantage of the ocp_addons approach:

  1. Clean boundary: Python <-> C++ conversion happens exactly once at the start (for read) or end (for write), not continuously during I/O
  2. No pointer aliasing: std::ostringstream owns its buffer entirely. No raw pointers into Python object internals.
  3. GIL-safe: The entire BinTools operation runs in pure C++ with no Python dependencies mid-stream
  4. Simple type conversion:
     • py::bytes -> std::string (pybind11 handles this cleanly, copies the data)
     • std::string -> py::bytes (single copy at the end via std::move)
  5. Native stream semantics: std::stringstream has well-defined, tested seek/tell behavior

• The Trade-off

  The pystreambuf approach was designed for streaming large data without loading everything into memory. But in practice:

  • Most CAD shapes are manageable in memory
  • The reliability cost of the complex bridging might outweigh the memory benefit

[adam-urbanczyk]

Thanks for the report, I can reproduce the issue. Do you know the exact root cause, or is the AI thing just some slop?

[bernhard-42]

My experience with the paid version (I have the Claude Max subscription) that can directly access the the source code is very good. Not that I trust it fully, by no means, but it found very tricky bugs for me and if you ensure to control every step is does, it can be really helpful.

So, I took claude code's proposal, compiled OCP and tested it. I have no idea whether this is efficient, but the bug is gone.
Maybe it helps you to fix it

diff --git a/pystreambuf.h b/pystreambuf.h
index 23480a1b..d913c4df 100644
--- a/pystreambuf.h
+++ b/pystreambuf.h
@@ -1,6 +1,16 @@
 /*
 Based on https://gist.github.com/asford/544323a5da7dddad2c9174490eb5ed06
 
+FIXED VERSION - Corrects seek position tracking bug that caused read failures
+with Python file objects (BytesIO, open()) when OCCT requested backward seeks.
+
+The bug: After a Python seek() was called, underflow() would add bytes read
+to pos_of_read_buffer_end_in_py_file without knowing the position had changed,
+causing position tracking to become corrupted.
+
+The fix: After calling Python seek(), invalidate the buffer and query the
+actual Python file position before the next read.
+
 Original license text
 ---------------------
 
@@ -176,7 +186,8 @@ class streambuf : public std::basic_streambuf<char>
       write_buffer(0),
       pos_of_read_buffer_end_in_py_file(0),
       pos_of_write_buffer_end_in_py_file(buffer_size),
-      farthest_pptr(0)
+      farthest_pptr(0),
+      read_buffer_valid(false)  // FIX: Track buffer validity
     {
       assert(buffer_size != 0);
       /* Some Python file objects (e.g. sys.stdout and sys.stdin)
@@ -237,12 +248,19 @@ class streambuf : public std::basic_streambuf<char>
         throw std::invalid_argument(
           "That Python file object has no 'read' attribute");
       }
+
+      // FIX: If buffer was invalidated (e.g., after seek), resync position from Python
+      if (!read_buffer_valid && !py_tell.is_none()) {
+        pos_of_read_buffer_end_in_py_file = py_tell().cast<off_type>();
+      }
+
       read_buffer = py_read(buffer_size);
       char *read_buffer_data;
       py::ssize_t py_n_read;
       if (PYBIND11_BYTES_AS_STRING_AND_SIZE(read_buffer.ptr(),
             &read_buffer_data, &py_n_read) == -1) {
         setg(0, 0, 0);
+        read_buffer_valid = false;  // FIX: Mark invalid
         throw std::invalid_argument(
           "The method 'read' of the Python file object "
           "did not return a string.");
@@ -250,6 +268,7 @@ class streambuf : public std::basic_streambuf<char>
       off_type n_read = (off_type)py_n_read;
       pos_of_read_buffer_end_in_py_file += n_read;
       setg(read_buffer_data, read_buffer_data, read_buffer_data + n_read);
+      read_buffer_valid = true;  // FIX: Mark valid after successful read
       // ^^^27.5.2.3.1 (4)
       if (n_read == 0) return failure;
       return traits_type::to_int_type(read_buffer_data[0]);
@@ -360,7 +379,14 @@ class streambuf : public std::basic_streambuf<char>
         }
         py_seek(off, whence);
         result = off_type(py_tell().cast<off_type>());
-        if (which == std::ios_base::in) underflow();
+
+        // FIX: After calling Python seek, invalidate the read buffer
+        // and reset position tracking. The next underflow() will resync.
+        if (which == std::ios_base::in) {
+          setg(0, 0, 0);  // Invalidate buffer pointers
+          pos_of_read_buffer_end_in_py_file = result;  // Reset to actual Python position
+          read_buffer_valid = false;  // Mark buffer as needing resync
+        }
       }
       return result;
     }
@@ -395,6 +421,9 @@ class streambuf : public std::basic_streambuf<char>
     // the farthest place the buffer has been written into
     char *farthest_pptr;
 
+    // FIX: Track whether read buffer position is valid
+    bool read_buffer_valid;
+
 
     bool seekoff_without_calling_python(
       off_type off,

[bernhard-42]

As expected, the initial fix wasn't efficient. I analysed the issue more:

[Py    5] read(1024) at 3072 -> got 1024 bytes
[pystreambuf  12] underflow() READ: got 1024 bytes, pos_of_read_buffer_end: 3072 -> 4096
[pystreambuf  13] seekoff() ENTER: off=3064, way=beg, which=in, pos_of_read_buffer_end=4096
[pystreambuf  14] seekoff_without_python: off=3064, pos_of_buffer_end=4096
[pystreambuf  15]   buffer logical range: [3072, 4096), seeking to file pos 3064
[pystreambuf  16]   -> OUTSIDE buffer, returning false (buf_sought=51651182616 not in [buf_begin, upper_bound))
[pystreambuf  17] seekoff() PYTHON PATH: seekoff_without_calling_python returned false
[pystreambuf  18] seekoff() calling py_seek(3064, 0), original off=3064
[Py    6] seek(3064, 0) from 4096 -> 3064
[Py    7] tell() -> 3064
[pystreambuf  19] seekoff() py_tell returned 3064, now calling underflow()
[pystreambuf  20] underflow() ENTER: pos_of_read_buffer_end=4096, buffer_size=1024
[Py    8] read(1024) at 3064 -> got 1024 bytes

Python suddenly comes with a seek command and works with position 3064 now.
But in C++, the code still assumes 4096 as if Python would have read 1024 bytes.

It turns out, it is sufficient to just change

363c363,366
<         if (which == std::ios_base::in) underflow();
---
>         if (which == std::ios_base::in) {
>           pos_of_read_buffer_end_in_py_file = result;  // FIX: Reset to actual position before refilling buffer
>           underflow();
>         }
586d588
<

By setting pos_of_read_buffer_end_in_py_file = result before calling underflow(), the position tracker is reset to the actual Python file position. Then when underflow() adds n_read, it calculates correctly. The fix must go after py_tell() (to get the actual position) and before underflow() (so the addition is correct).

[adam-urbanczyk]

Yeah, I came independently to the same conclusion, so that is somewhat reassuring. I'm still thinking about removing those classes completely.

[bernhard-42]

CAD objects fit into memory, so I don't think a capability of streaming large data is really needed. But input from Python buffer and output to Python buffer is a feature I wouldn't want to miss. I use it for example to compute a cache key for CAD objects in ocp-tessellate (BinTools.Write_s to memory is very fast).

The easiest option is to apply the above fix since it's simple. However, I think one could also replace in pystreambuf.h the classes istream (so that the input stream reads all data from a Python file object upfront) and ostream (so that the output stream buffers data and writes to Python file object on destruction). That would simplify the logic and create a clear boundary between C++ logic and Python logic.