Only Use Result From get_helper_prototype when valid#1050
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUnmarshal now uses a prototype-derived name when available instead of throwing; Linux helper lookup ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ir/unmarshal.cpp`:
- Around line 706-709: The code calls
info.platform->get_helper_prototype(inst.imm) twice and discards the validated
pointer; replace the second lookup by assigning the already-checked value: set
name = name_from_helper_prototype when name_from_helper_prototype is truthy
instead of calling get_helper_prototype(inst.imm) again. Update the block around
name_from_helper_prototype, get_helper_prototype, inst.imm, and name to reuse
the first result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc152546-85f9-40d1-8e0b-a1633a3e5ff0
📒 Files selected for processing (1)
src/ir/unmarshal.cpp
|
First, thank you for doing such awesome work on this verifier! I work with @Alan-Jowett and we use the verifier in our uBPF implementation. We have a configuration for fuzzing that does not do any meaningful work in the platform abstraction functions (e.g., Another potential fix for the issue is for us to make sure that we Thank you, again, for the great work!! |
hawkinsw
force-pushed
the
get_helper_prototype_use_fix
branch
from
d7fe4d9 to
fc23bfd
Compare
hawkinsw
force-pushed
the
get_helper_prototype_use_fix
branch
from
fc23bfd to
c1a39ad
Compare
|
Also, I am not 100% sure of the semantics that you are looking for here, but it seems like the conditional around this code might be backward. I see if (!info.platform->is_helper_usable(inst.imm)) {and the code being guarded then attempts to use the helper. It's possible that is the real issue here?! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ir/unmarshal.cpp (1)
493-496: 🧹 Nitpick | 🔵 TrivialInvariant assumption:
is_helper_usable(n)→get_helper_prototype(n).name != nullptrLine 496 assigns
proto.nameto astd::stringwithout a null check. This is safe given the current control flow (only called afteris_helper_usablereturns true at line 703), but the soundness relies on platform implementations maintaining this invariant.Per coding guidelines about encoding assumptions explicitly, consider adding an assertion or defensive check:
Suggested defensive assertion
auto makeCall(const int32_t imm) const -> Call { const EbpfHelperPrototype proto = info.platform->get_helper_prototype(imm); + assert(proto.name != nullptr && "get_helper_prototype returned null name for usable helper"); Call res; res.func = imm; res.name = proto.name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ir/unmarshal.cpp` around lines 493 - 496, The code assigns proto.name (from EbpfHelperPrototype returned by get_helper_prototype) to a std::string without checking for null, relying on the implicit invariant guaranteed by is_helper_usable; add an explicit defensive check or assertion to encode that invariant: validate that proto.name is non-null (or assert is_helper_usable(imm) / proto.name != nullptr) before assigning to res.name and handle the failure path (e.g., log/error/abort) so the contract is enforced at runtime; reference get_helper_prototype, EbpfHelperPrototype::name, is_helper_usable, and res.name when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/ir/unmarshal.cpp`:
- Around line 493-496: The code assigns proto.name (from EbpfHelperPrototype
returned by get_helper_prototype) to a std::string without checking for null,
relying on the implicit invariant guaranteed by is_helper_usable; add an
explicit defensive check or assertion to encode that invariant: validate that
proto.name is non-null (or assert is_helper_usable(imm) / proto.name != nullptr)
before assigning to res.name and handle the failure path (e.g., log/error/abort)
so the contract is enforced at runtime; reference get_helper_prototype,
EbpfHelperPrototype::name, is_helper_usable, and res.name when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d86abd3a-38bc-44d3-82d1-772b92a1bd68
📒 Files selected for processing (2)
src/ir/unmarshal.cppsrc/linux/gpl/spec_prototypes.cpp
Would be simpler to read: if (info.platform->is_helper_usable(inst.imm)) {
return makeCall(inst.imm);
}
// then your code |
That was the code that already existed. I was just pointing out that the existing code seems to be wrong. If you agree that the code (as it is currently) is wrong, then I would be more than happy to fix it! |
hawkinsw
force-pushed
the
get_helper_prototype_use_fix
branch
from
c1a39ad to
c3d861a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ir/unmarshal.cpp`:
- Around line 703-714: Centralize the null-name guard by moving the logic that
checks for a missing helper prototype name into makeCall() so callers like
makeJmp() and any direct uses of makeCall()/make_call() never receive a Call
with an empty or null name; specifically, inside makeCall() check the result of
info.platform->get_helper_prototype(inst.imm) (or its proto.name) and if missing
return a Call with is_supported=false and unsupported_reason set, otherwise
assign proto.name to Call::name and mark supported; remove the duplicate
fallback in the makeJmp() path and ensure get_helper_prototype() remains
unchanged unless you convert it to return an optional/explicit error type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f8a88b6-cc0e-4124-b473-4e108544f5a3
📒 Files selected for processing (2)
src/ir/unmarshal.cppsrc/linux/gpl/spec_prototypes.cpp
hawkinsw
force-pushed
the
get_helper_prototype_use_fix
branch
2 times, most recently
from
ed5dea9 to
a362bb9
Compare
In the case where the result EbpfHelperPrototype's name is null, do not attempt to use it. Also, remove try/catch protection and make sure that existing users of the get_helper_prototype platform abstraction function do not rely on it. Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
hawkinsw
force-pushed
the
get_helper_prototype_use_fix
branch
from
a362bb9 to
2171cb5
Compare
|
Okay! Let me know what you think of the PR in its current state. |
|
Thanks for accepting, @elazarg !! I hope I can contribute more in the future!! It's great to work with people like you! |
|
@hawkinsw you're welcome. Thanks for the PR! |
2 participants
In the case where the result EbpfHelperPrototype's name is null, do not attempt to use it.
Summary by CodeRabbit