Skip to content

Bug reports and bug fixes - 2nd slack - 3 more IDEExtendedTaintAnalysis false-negatives #846

Open

Description

@pserwa-exida

Hi Fabian!

Follow-up to the issue I opened earlier ("Several build-compat + taint/path-sensitivity fixes") — same fork, same toolchain, no new context needed. After that batch landed we pointed the analysis at a gnarlier real activity (an obstacle/polyline extractor — a vtable-bearing class whose members sit behind the vtable pointer, a confidence value built by OR-ing two fields together, and a whole-struct message copy). Three more real flows went missing on it, all in IDEExtendedTaintAnalysis, all false negatives. We've fixed all three on the same branch and I wanted to write them up while they were fresh.

Same as last time: each commit message is a standalone write-up, and I've pulled a minimal fixture out of our suite for each. All three fixtures are clean (no FP, no FN) on the combo we actually ship — --solver=ide-xtaint --alias-analysis=union-find. Happy to split into separate PRs whenever you want them.

The three

  1. Taint on this->member.<field> is dropped at the call edge when the member is at a non-zero offset (false negative). A free function (or any callee) that takes &this->member by reference/pointer — the obstacles::FanToPolylines(const Fan& in, Polylines& out) shape — doesn't receive the taint when member sits at a non-zero byte offset inside the object. The fact about this->member.field arrives on the callee's formal still carrying the member's displacement, so the callee reads its parameter at the wrong byte and the flow vanishes. The reason it hid for so long: if the member happens to be at offset 0 (first field, no vtable), the displacement is 0 and the transfer is correct — so every small reproducer that put the struct first "worked", and only a class with a vtable pointer (member at byte 8) or a leading scalar exposed it. Cause is in AbstractMemoryLocationFactory::withTransferToImpl: when it rebases a caller fact onto the callee formal it wasn't subtracting the &this->member displacement. Fix subtracts it (base-match + single leading member offset; falls back to the existing leaf-clamp otherwise).
    Severity: recall (missed real flow through a member-pointer converter).
    Fix → https://gitlab.com/exida-oss/stig/phasar/-/commit/3ce82bf32eb169fe95146c97490b0a337611273c
    Triggers → activity_member_struct_external_kernel_offset.cpp (a leading long pushes the converted members off offset 0 — same effect as a vtable pointer, without needing one)

  2. Taint doesn't flow through value-combining instructions (false negative). When a tainted value is combined with another through a binary op / icmp / select and the result is stored, the result loses the taint. Ours was a confidence byte: out.hoo = (uint8_t)classification_type | (uint8_t)position_x; — classification_type is tainted, so out.hoo should be too, but it came out clean. The factory that builds access-path facts in getNormalFlowFunction only follows single-operand derivations (load / cast / GEP — there's one access path to chase). A binary op / compare / select has two value operands and no single access path, so the result was treated as a fresh opaque base and never inherited operand taint. PHINode is already special-cased to union its incoming facts; this is the same idea for the value-combiners. Fix generates a fact for the instruction result when any operand matches the incoming source. One wrinkle worth calling out: we exclude bit-shifts. Struct returns of two i32s get packed into an i64 and unpacked with trunc(lshr(packed, 32)), and if shifts propagate taint that packing bleeds one returned field onto its sibling (a fresh FP). Shifts relocate bits rather than combine two tainted inputs, so dropping them is principled, not a fudge.
    Severity: recall (any tainted value merged arithmetically/bitwise before a sink).
    Fix → https://gitlab.com/exida-oss/stig/phasar/-/commit/4a021fe490a31475ee85ccacfb7633edcef19ef4
    Triggers → value_combine_or.cpp (the operands are loaded struct fields, which is the case the access-path rebasing reaches and the shape the real activity has — a pure call-result merge is a separate, value-representation issue and we left that one as a known FN)

  3. Whole-struct assignment lowered to llvm.memcpy drops per-field taint (false negative). dst = src for a struct big enough that the compiler emits a single llvm.memcpy instead of scalar stores — a field tainted in src doesn't reach the same field of dst. This one was painful in practice: it's why our activity had a hand-expanded fan_ = message.data; into 14 explicit per-field stores as a workaround. The MemTransferInst handling in getSummaryFlowFunction routed the copy through the generic store path, which appended the source field's offset path onto the destination buffer pointer and produced a non-canonical, "lumpy" offset vector (something like {this,[0,4,0]}) that a later flattened load of the same field ({this,[4,0]}) couldn't match — so the copy silently lost field taint. Fix rebases each source-buffer field onto the destination with withTransferFrom, which yields the canonical flattened offset and matches the load; base-match works at any nesting depth.
    Severity: recall (whole-struct / message copies — extremely common in our domain).
    Fix → https://gitlab.com/exida-oss/stig/phasar/-/commit/8f9d431392ab94e3f751f0b75770dbc2349189e1
    Triggers → struct_copy_memcpy.cpp (the struct carries borders[256] so the assignment stays a memcpy at -O2 rather than being scalarised — drop the array and it gets optimised into per-field stores and the bug hides)

One instrumentation commit, optional

Same as the XSTORE dump in the first issue: while chasing #10/#11 we added an env-gated dump of the call/return boundary facts (STIG_CALL_DEBUG=1 prints XCALL / XRET lines). It changes nothing when the env var is unset and we found it handy enough to keep — take it or leave it.

On the combos

I only assert these fixtures on ide-xtaint × union-find, since that's the configuration we focus on (although svf-vta is also a contender). They're field-sensitivity / access-path problems, so the field-insensitive solvers (ifds-taint) and the field-insensitive alias backend over- or under-approximate them in the usual ways regardless of these patches — we're not claiming to fix those, just flagging that if you run the fixtures across the full solver/alias matrix you'll see the expected approximation on the other cells, not a regression.

Reproducing

Same as before — the three .cpp files are linked inline above, in our public repo next to the first issue's fixtures.

Regards,
Piotr

==
Piotr Serwa
Principal Safety Engineer
exida

Activity

  1. fabianbs96 commented on Jun 19, 2026

    @fabianbs96
    Member

    Hi @pserwa-exida, thanks for your requests.
    I will look into them. Given the number of issues, it will probably take me a while to review.

    Regarding 6. (LLVM version in find_package): this is a problem that I have already experienced myself. For me, it helped to specify the minor version as well, i.e., -DPHASAR_LLVM_VERSION=18.1.

  2. fabianbs96 commented on Jun 24, 2026

    @fabianbs96
    Member

    Hi @pserwa-exida, here some intermediate update:

    I looked through your issues, and I have confirmed 1., 2., 3., 9., 10., 11., and 12.. The fixes look correct; however, I would reduce the AI-comments to be less verbose.

    For 4./5. I have reproduced the bug, not sure yet whether the proposed fix fixes the root-cause, or just a symptom.

    For 6., specifying the minor version as well should work (see previous comment).

    For 7. not sure what find_package variant is the better one here, but it should probably match the version used in the CMakeLists.txt

    For 8.: CTAD is essential for the usability of the PBMixin, so I would really like to keep it. I could not reproduce the internal compiler error with my clang-22 installation. Have you tried whether explicit deduction guide helps?

    // maybe sth. like this:
    template <typename FirstT, typename SecondT>
    PBMixin(FirstT, SecondT) -> PBMixin<FirstT, SecondT>;

    For the already confirmed bugs, you are welcome to create pull requests. I would suggest creating multiple ones, perhaps grouped by topic.
    Again, thanks for raising these issues and giving these detailed descriptions! They were increadibily useful for reproducing the bugs!

  3. pserwa-exida commented on Jul 21, 2026

    @pserwa-exida
    Author

    Hi Fabian,

    thanks for confirming items 1, 2, 3, 9, 10, 11, and 12.

    I agree that the comments should be shortened before upstream submission. They currently contain more diagnostic context than should remain in the final implementation.

    For 4 and 5, I understand the concern that the proposed changes might address a symptom rather than the underlying access-path issue. We currently need these patches locally because the focused regression cases and our end-to-end obstacle/polyline analysis depend on them. Before preparing PRs, I would reduce these cases further into LLVM-level tests around AbstractMemoryLocation construction and the store/sink fact transfer. That should make it easier to determine whether the current correction is at the right layer. Or just rework/rethink based on your feedback.

    For 6, specifying the minor version does work. Fedora 44 LLVMConfigVersion.cmake reports LLVM 18.1.8, and find_package(LLVM 18) is effectively checked as 18.0 and rejected. find_package(LLVM 18.1) accepts the different 18.1.x patch releases used by Ubuntu and Fedora. Avoiding an exact patch version was the important part for our two distributions. So we just restrict to 18.x, but you may want to have a more stict

    In our local branch I have also tested another variant: LLVM_DIR selects the installation, followed by an explicit LLVM_VERSION_MAJOR check. This permits distro patch differences while still rejecting the wrong LLVM major.

    For 7, I agree that the installed phasarConfig.cmake should use the same LLVM-version policy as the main build. The separate Z3 problem is the exported target identity: depending on how Z3 is found, the available target is either z3 or z3::libz3.

    For 8, our original workaround is already committed on our GitLab branch:
    https://gitlab.com/exida-oss/stig/phasar/-/commit/d7c131e60ceb89e3c5d5175730c78cd1c8697e15

    Following your suggestion, I also tested an explicit deduction guide:

    template <typename FirstT, typename SecondT>
    PBMixin(FirstT, SecondT) -> PBMixin<FirstT, SecondT>;

    This lets us restore the original CTAD call sites and builds successfully with Clang 22.1.7. We will update the existing patch to use this approach and submit it as another PR.

    I will try to find time to prepare the pull requests. The fixes are currently available on our public GitLab clone - I will share them over another github issue/message. I may mirror or move the fork to GitHub to make the PR workflow easier. I can also reduce the comments and split the changes by topic, but I cannot promise a date at the moment.

    I have found a few additional IDEExtendedTaintAnalysis bugs since the previous submission. The fixes and failing-first regression cases are already available on our branch. I will submit them as a separate issue.

    Regards,
    Piotr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions