Skip to content

engine: refuse any blob the kernels cannot run safely - #3

Merged
widgetii merged 2 commits into
masterfrom
harden-parse
Oct 11, 2026
Merged

widgetii merged 2 commits into
masterfrom
harden-parse

Conversation

@widgetii

Copy link
Copy Markdown
Member

From the review of majestic's copy of this engine (widgetii/majestic#1030): the model is a file on the camera, so a malformed one must be refused when it is opened, not trusted into a kernel.

parse() now requires:

  • an even input size, because NV21 carries chroma in pairs;
  • int8 zero points throughout;
  • a 1×1 convolution with stride 1 and no padding, as its kernel assumes, and padding below the kernel size for the others;
  • requantization shifts in range for the add as well as the convolution;
  • every op to read a tensor an earlier op produced as int8, and to write a new one;
  • float detection heads on matching grids;
  • all activations together within 8 MiB, counted in 64 bits.

The decode bounds each box distance before it becomes an int. The lab-only profiling code is removed.

Unchanged outputs: a full scripts/build.sh produces the same person.pdnet and person.json (identical SHA256SUMS) and the same points.md. The host and ARMv7 NEON builds are still bit-identical to the reference, and the ARM build finds exactly the host's boxes on all 3000 crops. majestic's tests/detect/pdnet.c exercises each rejection.

From the review of majestic's copy. The model is a file on the camera,
so a malformed one must be refused at open, not trusted into a kernel.
parse() now requires:
- an even input (NV21 reads chroma in pairs) and int8 zero points;
- a 1x1 convolution with stride 1 and no padding, as its kernel assumes,
  and padding below the kernel size;
- requant shifts in range for the add as well as the convolution;
- every op to read a tensor an earlier op produced as int8 and write a
  new one, so execution order and storage are known;
- float detection heads on matching grids;
- all activations together within 8 MiB, counted in 64 bits.
The decode bounds every box distance before it becomes an int, and the
lab-only profiling code is gone.

The release outputs are unchanged: a full build gives the same
person.pdnet, person.json and points.md, with the host and ARM NEON
builds still identical to the reference.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Reject unsafe model blobs before running detector kernels

🐞 Bug fix 🕐 20-40 Minutes

Grey Divider

AI Description

• Reject malformed models at open time when tensor types, operation parameters, or activation sizes
 are unsafe.
• Bound decoded box distances before integer conversion and remove lab-only profiling code.
• Reported build checks show unchanged release artifacts and matching host and ARM detections.
Diagram

graph TD
  Blob["Model blob"] --> Parse{"Parse valid?"} --> Buffers["Typed buffers"] --> Kernels["Int8 kernels"] --> Heads["Float heads"] --> Decode["Bounded decode"] --> Boxes["Person boxes"]
  Parse --> Reject["Reject blob"]
Loading
High-Level Assessment

Keep validation in parse(): it rejects unsafe files before allocation or kernel execution, while tracking tensor kinds in operation order matches how the engine runs. Runtime checks would defer failures until detection; a buffer-liveness planner would add complexity without a clear need under the 8 MiB activation cap.

Files changed (1) +67 / -61

Bug fix (1) +67 / -61
pdnet.cValidate model execution safety at open time +67/-61

Validate model execution safety at open time

• The parser now checks even NV21 dimensions, int8 zero points, convolution constraints, requantization shifts, tensor production order and types, matching float detection heads, and an 8 MiB aggregate activation limit. Allocation uses the validated tensor type; decoding bounds box distances before integer conversion. Lab-only per-operation profiling code is removed.

engine/pdnet.c

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Large box distances change detections ✓ Resolved
Description
pdnet_detect() caps every regression distance at 1024 stride units before calculating box edges,
even when the original distance would produce valid integer coordinates. A finite head output of
1025 at stride 8, for example, now yields a smaller box, which can change overlap matching and the
detections used to score presets.
Code

engine/pdnet.c[811]

+                    r[s4] = fminf(fmaxf(0.0f, tr->f[(size_t)i * 4 + s4]), 1024.0f);
Evidence
The new clamp feeds all four box-edge calculations; the previous calculations used each nonnegative
distance without an upper cap. The preset evaluation matches emitted boxes by
intersection-over-union, so a changed box can change its classification.

engine/pdnet.c[803-819]
tools/points.py[27-52]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The fixed 1024-unit cap changes safely representable decoded boxes, rather than only preventing unsafe integer conversions.
## Fix Focus Areas
- engine/pdnet.c[806-819]
## Recommended Fix
Bound decoded coordinates according to the integer range and the subsequent box-width calculations, preserving the existing result whenever those calculations are safe.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can tweak Display settings with a live preview to see your comment before it ships

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread engine/pdnet.c Outdated
From review: a 1024-stride cap could shorten a box that still fits an int.
1e6 strides at a stride of at most 64 keeps every edge inside an int and
changes no box that fits one.
@widgetii
widgetii merged commit 3decf6e into master Oct 11, 2026
3 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.

1 participant