github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-04-27 12:18:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Fix PathData corruption root causes across WASM and CLJS

Browse Source 
Replace unsafe std::mem::transmute calls in Rust WASM path code with
validated TryFrom conversions to prevent undefined behavior from invalid
enum discriminant values. This was the most likely root cause of the
"No matching clause: -19772" production crash -- corrupted bytes flowing
through transmute could produce arbitrary invalid enum variants.

Fix byteOffset handling throughout the CLJS PathData serialization
pipeline. DataView instances created via buf/slice carry a non-zero
byteOffset, but from-bytes, transit write handler, -write-to,
buf/clone, and buf/equals? all operated on the full underlying
ArrayBuffer, ignoring offset and length. This could silently produce
PathData with incorrect size or content.

Rust changes (render-wasm):
- RawSegmentData: From<[u8; N]> -> TryFrom<[u8; N]> with discriminant
  validation (must be 0x01-0x04) before transmuting
- RawBoolType: From<u8> -> TryFrom<u8> with explicit match on 0-3
- Add #[wasm_error] to set_shape_path_content, current_to_path,
  convert_stroke_to_path, and set_shape_bool_type so panics are caught
  and routed through the WASM error protocol instead of crashing
- set_shape_path_content: replace .expect() with proper Result/? error
  propagation per segment
- Remove unused From<BytesType> bound from SerializableResult trait

CLJS changes (common):
- from-bytes: use DataView.byteLength instead of ArrayBuffer.byteLength
  for DataView inputs; preserve byteOffset/byteLength when converting
  from Uint8Array, Uint32Array, and Int8Array
- Transit write handler: construct Uint8Array with byteOffset and
  byteLength from the DataView, not the full backing ArrayBuffer
- -write-to: same byteOffset/byteLength fix
- buf/clone: copy only the DataView byte range using Uint8Array with
  proper offset, not Uint32Array over the full ArrayBuffer
- buf/equals?: compare DataView byte ranges using Uint8Array with
  proper offset, not the full backing ArrayBuffers

Frontend changes:
- shape-to-path, stroke-to-path, calculate-bool*: wrap WASM call and
  buffer read in try/catch to ensure mem/free is always called, even
  when an exception occurs between the WASM call and the free call

Signed-off-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
Andrey Antukh 2026-04-08 12:38:28 +00:00 committed by Belén Albeza
parent 2eaa2dc807
commit 92de9ed258
4 changed files with 226 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
common/src/app/common/buffer.cljc
View File

@ -217,10 +217,12 @@
(let [buffer (ByteBuffer/wrap dst)]
(.order buffer ByteOrder/LITTLE_ENDIAN)))
:cljs
(let [buffer' (.-buffer ^js/DataView buffer)
src-view (js/Uint32Array. buffer')
dst-buff (js/ArrayBuffer. (.-byteLength buffer'))
dst-view (js/Uint32Array. dst-buff)]
(let [src-off (.-byteOffset ^js/DataView buffer)
src-len (.-byteLength ^js/DataView buffer)
src-buf (.-buffer ^js/DataView buffer)
src-view (js/Uint8Array. src-buf src-off src-len)
dst-buff (js/ArrayBuffer. src-len)
dst-view (js/Uint8Array. dst-buff)]
(.set dst-view src-view)
(js/DataView. dst-buff))))
@ -239,12 +241,15 @@
^ByteBuffer buffer-b)
:cljs
(let [buffer-a (.-buffer buffer-a)
buffer-b (.-buffer buffer-b)]
(if (= (.-byteLength buffer-a)
(.-byteLength buffer-b))
(let [cb (js/Uint32Array. buffer-a)
ob (js/Uint32Array. buffer-b)
(let [len-a (.-byteLength ^js/DataView buffer-a)
len-b (.-byteLength ^js/DataView buffer-b)]
(if (= len-a len-b)
(let [cb (js/Uint8Array. (.-buffer ^js/DataView buffer-a)
(.-byteOffset ^js/DataView buffer-a)
len-a)
ob (js/Uint8Array. (.-buffer ^js/DataView buffer-b)
(.-byteOffset ^js/DataView buffer-b)
len-b)
sz (alength cb)]
(loop [i 0]
(if (< i sz)

30
common/src/app/common/types/path/impl.cljc
View File

@ -390,10 +390,10 @@
;; NOTE: we still use u8 because until the heap refactor merge
;; we can't guarrantee the alignment of offset on 4 bytes
(assert (instance? js/ArrayBuffer into-buffer))
(let [buffer' (.-buffer ^js/DataView buffer)
size (.-byteLength buffer')
(let [size (.-byteLength ^js/DataView buffer)
src-off (.-byteOffset ^js/DataView buffer)
mem (js/Uint8Array. into-buffer offset size)]
(.set mem (js/Uint8Array. buffer'))))
(.set mem (js/Uint8Array. (.-buffer ^js/DataView buffer) src-off size))))
ITransformable
(-transform [this m]
@ -635,19 +635,27 @@
nil))
(instance? js/DataView buffer)
(let [buffer' (.-buffer ^js/DataView buffer)
size (.-byteLength ^js/ArrayBuffer buffer')
count (long (/ size SEGMENT-U8-SIZE))]
(let [size (.-byteLength ^js/DataView buffer)
count (long (/ size SEGMENT-U8-SIZE))]
(PathData. count buffer (weak/weak-value-map) nil))
(instance? js/Uint8Array buffer)
(from-bytes (.-buffer buffer))
(let [ab (.-buffer buffer)
offset (.-byteOffset buffer)
size (.-byteLength buffer)]
(from-bytes (js/DataView. ab offset size)))
(instance? js/Uint32Array buffer)
(from-bytes (.-buffer buffer))
(let [ab (.-buffer buffer)
offset (.-byteOffset buffer)
size (.-byteLength buffer)]
(from-bytes (js/DataView. ab offset size)))
(instance? js/Int8Array buffer)
(from-bytes (.-buffer buffer))
(let [ab (.-buffer buffer)
offset (.-byteOffset buffer)
size (.-byteLength buffer)]
(from-bytes (js/DataView. ab offset size)))
:else
(throw (js/Error. "invalid data provided")))))
@ -733,7 +741,9 @@
:class PathData
:wfn (fn [^PathData pdata]
(let [buffer (.-buffer pdata)]
#?(:cljs (js/Uint8Array. (.-buffer ^js/DataView buffer))
#?(:cljs (js/Uint8Array. (.-buffer ^js/DataView buffer)
(.-byteOffset ^js/DataView buffer)
(.-byteLength ^js/DataView buffer))
:clj (.array ^ByteBuffer buffer))))
:rfn from-bytes})

146
common/test/common_tests/types/path_data_test.cljc
View File

@ -1272,3 +1272,149 @@
(let [segs (vec (:content result))
curve-segs (filter #(= :curve-to (:command %)) segs)]
(t/is (pos? (count curve-segs))))))
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; TYPE CODE CONSISTENCY TESTS (regression for move-to/line-to swap bug)
;;
;; These tests ensure that all binary reader code paths agree on the
;; mapping: 1=move-to, 2=line-to, 3=curve-to, 4=close-path.
;;
;; The bug was that `impl-walk`, `impl-reduce`, and `impl-lookup` had
;; type codes 1 and 2 swapped (1→:line-to, 2→:move-to) while
;; `read-segment`, `from-plain`, and `to-string-segment*` had the
;; correct mapping. This caused subtle mismatches in operations like
;; `get-subpaths`, `get-points`, `get-handlers`, etc.
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
(t/deftest type-code-walk-consistency
(t/testing "impl-walk produces same command types as read-segment (via vec)"
(let [pdata (path/content sample-content)
;; read-segment path: produces {:command :keyword ...} maps
seq-types (mapv :command (vec pdata))
;; impl-walk path: collects type keywords
walk-types (path.impl/-walk pdata
(fn [type _ _ _ _ _ _] type)
[])]
;; Both paths must agree on the command types
(t/is (= seq-types walk-types))
;; Verify the actual expected types
(t/is (= [:move-to :line-to :curve-to :close-path] seq-types))
(t/is (= [:move-to :line-to :curve-to :close-path] walk-types)))))
(t/deftest type-code-reduce-consistency
(t/testing "impl-reduce produces same command types as read-segment (via vec)"
(let [pdata (path/content sample-content)
;; read-segment path
seq-types (mapv :command (vec pdata))
;; impl-reduce path: collects [index type] pairs
reduce-types (path.impl/-reduce
pdata
(fn [acc index type _ _ _ _ _ _]
(conj acc type))
[])]
(t/is (= seq-types reduce-types))
(t/is (= [:move-to :line-to :curve-to :close-path] reduce-types)))))
(t/deftest type-code-lookup-consistency
(t/testing "impl-lookup produces same command types as read-segment for each index"
(let [pdata (path/content sample-content)
seg-count (count pdata)]
(doseq [i (range seg-count)]
(let [;; read-segment path
seg-type (:command (nth pdata i))
;; impl-lookup path
lookup-type (path.impl/-lookup
pdata i
(fn [type _ _ _ _ _ _] type))]
(t/is (= seg-type lookup-type)
(str "Mismatch at index " i
": read-segment=" seg-type
" lookup=" lookup-type)))))))
(t/deftest type-code-get-points-uses-walk
(t/testing "get-points (via impl-walk) excludes close-path and includes move-to/line-to/curve-to"
(let [pdata (path/content sample-content)
points (path.segment/get-points pdata)
;; Manually extract points from read-segment (via vec),
;; skipping close-path
expected-points (->> (vec pdata)
(remove #(= :close-path (:command %)))
(mapv #(gpt/point
(get-in % [:params :x])
(get-in % [:params :y]))))]
(t/is (= expected-points points))
;; Specifically: 3 points (move-to, line-to, curve-to)
(t/is (= 3 (count points))))))
(t/deftest type-code-get-subpaths-uses-reduce
(t/testing "get-subpaths (via reduce) correctly identifies move-to to start subpaths"
(let [;; Content with two subpaths: move-to + line-to + close-path, then move-to + line-to
two-subpath-content
[{:command :move-to :params {:x 0.0 :y 0.0}}
{:command :line-to :params {:x 10.0 :y 0.0}}
{:command :close-path :params {}}
{:command :move-to :params {:x 20.0 :y 20.0}}
{:command :line-to :params {:x 30.0 :y 30.0}}]
pdata (path/content two-subpath-content)
subpaths (path.subpath/get-subpaths pdata)]
;; Must produce exactly 2 subpaths (one per move-to)
(t/is (= 2 (count subpaths)))
;; First subpath starts at (0,0)
(t/is (= (gpt/point 0.0 0.0) (:from (first subpaths))))
;; Second subpath starts at (20,20)
(t/is (= (gpt/point 20.0 20.0) (:from (second subpaths)))))))
(t/deftest type-code-get-handlers-uses-reduce
(t/testing "get-handlers (via impl-reduce) correctly identifies curve-to segments"
(let [pdata (path/content sample-content)
handlers (path.segment/get-handlers pdata)]
;; sample-content has one curve-to at index 2
;; The curve-to's :c1 handler belongs to the previous point (line-to endpoint)
;; The curve-to's :c2 handler belongs to the curve-to endpoint
(t/is (some? handlers))
(let [line-to-point (gpt/point 439.0 802.0)
curve-to-point (gpt/point 264.0 634.0)]
;; line-to endpoint should have [2 :c1] handler
(t/is (some #(= [2 :c1] %) (get handlers line-to-point)))
;; curve-to endpoint should have [2 :c2] handler
(t/is (some #(= [2 :c2] %) (get handlers curve-to-point)))))))
(t/deftest type-code-handler-point-uses-lookup
(t/testing "get-handler-point (via impl-lookup) returns correct values"
(let [pdata (path/content sample-content)]
;; Index 0 is move-to (480, 839) — not a curve, so any prefix
;; returns the segment point itself
(let [pt (path.segment/get-handler-point pdata 0 :c1)]
(t/is (= (gpt/point 480.0 839.0) pt)))
;; Index 2 is curve-to with c1=(368,737), c2=(310,681), point=(264,634)
(let [c1-pt (path.segment/get-handler-point pdata 2 :c1)
c2-pt (path.segment/get-handler-point pdata 2 :c2)]
(t/is (= (gpt/point 368.0 737.0) c1-pt))
(t/is (= (gpt/point 310.0 681.0) c2-pt))))))
(t/deftest type-code-all-readers-agree-large-content
(t/testing "all binary readers agree on types for a large multi-segment path"
(let [pdata (path/content sample-content-large)
seg-count (count pdata)
;; Collect types from all four code paths
seq-types (mapv :command (vec pdata))
walk-types (path.impl/-walk pdata
(fn [type _ _ _ _ _ _] type)
[])
reduce-types (path.impl/-reduce
pdata
(fn [acc _ type _ _ _ _ _ _]
(conj acc type))
[])
lookup-types (mapv (fn [i]
(path.impl/-lookup
pdata i
(fn [type _ _ _ _ _ _] type)))
(range seg-count))]
;; All four must be identical
(t/is (= seq-types walk-types))
(t/is (= seq-types reduce-types))
(t/is (= seq-types lookup-types))
;; Verify first and last entries specifically
(t/is (= :move-to (first seq-types)))
(t/is (= :close-path (last seq-types))))))

78
frontend/src/app/render_wasm/api.cljs
View File

@ -1541,35 +1541,43 @@
(defn shape-to-path
[id]
(use-shape id)
(let [offset (-> (h/call wasm/internal-module "_current_to_path")
(mem/->offset-32))
heap (mem/get-heap-u32)
length (aget heap offset)
data (mem/slice heap
(+ offset 1)
(* length path.impl/SEGMENT-U32-SIZE))
content (path/from-bytes data)]
(mem/free)
content))
(try
(let [offset (-> (h/call wasm/internal-module "_current_to_path")
(mem/->offset-32))
heap (mem/get-heap-u32)
length (aget heap offset)
data (mem/slice heap
(+ offset 1)
(* length path.impl/SEGMENT-U32-SIZE))
content (path/from-bytes data)]
(mem/free)
content)
(catch :default cause
(mem/free)
(throw cause))))
(defn stroke-to-path
"Converts a shape's stroke at the given index into a filled path.
Returns the stroke outline as PathData content."
[id stroke-index]
(use-shape id)
(let [offset (-> (h/call wasm/internal-module "_convert_stroke_to_path" stroke-index)
(mem/->offset-32))
heap (mem/get-heap-u32)
length (aget heap offset)]
(if (pos? length)
(let [data (mem/slice heap
(+ offset 1)
(* length path.impl/SEGMENT-U32-SIZE))
content (path/from-bytes data)]
(mem/free)
content)
(do (mem/free)
nil))))
(try
(let [offset (-> (h/call wasm/internal-module "_convert_stroke_to_path" stroke-index)
(mem/->offset-32))
heap (mem/get-heap-u32)
length (aget heap offset)]
(if (pos? length)
(let [data (mem/slice heap
(+ offset 1)
(* length path.impl/SEGMENT-U32-SIZE))
content (path/from-bytes data)]
(mem/free)
content)
(do (mem/free)
nil)))
(catch :default cause
(mem/free)
(throw cause))))
(defn calculate-bool*
[bool-type ids]
@ -1582,17 +1590,21 @@
offset
(rseq ids))
(let [offset
(-> (h/call wasm/internal-module "_calculate_bool" (sr/translate-bool-type bool-type))
(mem/->offset-32))
(try
(let [offset
(-> (h/call wasm/internal-module "_calculate_bool" (sr/translate-bool-type bool-type))
(mem/->offset-32))
length (aget heap offset)
data (mem/slice heap
(+ offset 1)
(* length path.impl/SEGMENT-U32-SIZE))
content (path/from-bytes data)]
(mem/free)
content)))
length (aget heap offset)
data (mem/slice heap
(+ offset 1)
(* length path.impl/SEGMENT-U32-SIZE))
content (path/from-bytes data)]
(mem/free)
content)
(catch :default cause
(mem/free)
(throw cause)))))
(defn calculate-bool
[shape objects]