Skip to content

Fix a few loader crashes and silent bugs - #9

Merged
sinokadev merged 6 commits into
sinokadev:mainfrom
sirin-interact:fix/loader-crashes-and-lookat
Oct 27, 2026
Merged

sinokadev merged 6 commits into
sinokadev:mainfrom
sirin-interact:fix/loader-crashes-and-lookat

Conversation

@sirin-interact

Copy link
Copy Markdown
Contributor

Hi! I was poking around the engine and hit a handful of bugs, mostly in the Seno loader. All of these reproduce on 3b72610 (Ubuntu 24.04, gcc 13, ASan/UBSan on, Mesa llvmpipe for the GL parts). One commit per fix, so feel free to drop anything you don't want.

1. Out-of-range index in an embedded mesh segfaults

{"version": 8,
 "meshes": [{"vertices": [{"position": [0,0,0]}, {"position": [1,0,0]}, {"position": [0,1,0]}],
             "indices": [0, 1, 100000]}]}

loadSeno() crashes in calculateMeshTangents(), which does vertices[indices[i]] without a check. glTF primitives go through the same function with no validation either. Both loaders now call a small isValidTriangleList() first. Seno indices also have to be non-negative 32-bit ints now, since get<unsigned int>() quietly turned -1 or 4294967296 into some other index.

2. Short arrays read past the end

"position": [1] (or a 3-element rotation, a vertex without position, ...) goes through const json::operator[], which has no bounds check. Under ASan it's a heap-use-after-free. Without ASan I got type must be number, but is number, which is it reading garbage. Added readVec2/readVec3/readQuat helpers that throw a readable error instead.

3. A bad file wipes the scene that's already loaded

Load any valid scene, then loadSeno() a file with "objects": [{"model": 5}]. It returns false, but the old objects are gone, because clear() runs before anything is parsed. Everything is now built into locals and swapped in at the end.

4. Unknown camera type returns true with a null camera

"camera": {"type": "Perspective"} (typo) sets the camera to nullptr and reports success. The next renderScene() dereferences it. It's a load error now. I added the same check plus the whole-triangle rule to the editor's validate(), so the editor won't save a file the loader rejects.

5. Straight-down DirLight comes out at 45°

knot::DirLight sun;      // default direction (0,-1,0)
sun.getDirection();      // (0, -0.707, -0.707)

glm::quatLookAt(dir, +Y) breaks down when dir is vertical. This hits every Seno DirLight without a rotation, and Camera::lookAtTarget too (the benchmark camera looks straight down). New knot::lookRotation() only switches to a Z up when glm would actually break, so every other direction gives exactly the same rotation as before.

6. PbrMaterial setters don't change anything on screen

setAlbedoColor/setMetallic/setRoughness/setAo only update baseAlbedo etc., but the shader samples the 1x1 fallback textures built in the constructor. The setters now rewrite that texel in place (same texture id, binding restored, value clamped). If a real map is set they still don't touch it, which matches the "fallback" wording in the docs.

7. Timer ids are garbage

nextTimerTaskId had no initializer, so after() returned -1414812757 for me. It starts at 1 now.

Heads up

The loader is a bit stricter now. Arrays with extra components (e.g. an RGBA "color") used to be accepted with the extra values ignored; now they're an error. Nothing in the repo writes those: I checked the three assets/*.seno files, the Blender exporter, u2s.py and the editor, and they all still load.

Testing

  • Added the cases above to seno-editor/tests/loader_tests.cpp and document_tests.cpp. They fail on 3b72610 (the loader test dies under ASan on the short-array case) and pass here.
  • test_submesh now reads the albedo texel back after setAlbedoColor(). It fails on 3b72610 and passes here.
  • ctest for seno-editor passes with ASan/UBSan. I ran scene_render, gltf_scene, test_group and test_seno_gltf under xvfb with no new errors, and checked that every commit builds on its own. clang-format 20 doesn't want any changes.
  • Only tested with gcc on Linux, not MSVC or macOS.

Side note: CI doesn't build the seno-editor tests at the moment, so these won't run there. Might be worth hooking up at some point.

P.S. No star from me yet. I'm saving it for when the engine levels up, and this PR is me grinding a little XP for it.

nextTimerTaskId had no initializer, so after() and repeat() handed out
whatever was in memory (0xABABABAB in my test).
glm::quatLookAt(dir, +Y) is degenerate when dir is vertical. A default
DirLight (0,-1,0) came out as (0,-0.707,-0.707), and so did any Seno
DirLight without a rotation and Camera::lookAtTarget straight down.

Add knot::lookRotation() which only switches to a Z up when glm would
break, so every other direction gives exactly the same result as before.
An embedded mesh with an index >= vertex count segfaulted in
calculateMeshTangents(), and the glTF loader had the same gap. Both now
check isValidTriangleList() first. Seno indices also have to be
non-negative 32-bit integers instead of being silently wrapped.
loadSeno() called clear() before parsing, so a file that failed halfway
left an empty scene behind. Build everything into locals and swap at the
end instead.

Also:
- read fixed-size arrays with bounds checks; "position": [1] used to read
  past the end of the json array
- reject unknown camera types instead of returning true with a null
  camera that crashes on the next frame
setAlbedoColor/setMetallic/setRoughness/setAo only changed the base*
fields, but the shader samples the 1x1 fallback textures created in the
constructor, so nothing changed on screen. Rewrite the texel in place.
@sinokadev
sinokadev merged commit b985b24 into sinokadev:main Oct 27, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants