Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The new external model entries are consistent with existing C++ summaryModel conventions and are validated by targeted external-models harness tests.
Pull request overview
Adds new C++ dataflow summary models for Bloomberg BDE bal* codec APIs so taint can traverse serialization/deserialization boundaries (streams ↔ objects), with library-test coverage via the external-models harness.
Changes:
- Introduces a new external summary model file for
BloombergLP::balber,BloombergLP::baljsn, andBloombergLP::balxmlencode/decode methods. - Adds a new external-models test stub (
bal.cpp) covering stream→object decode, object→stream encode, and a round-trip case (plus a documented no-flow field-access limitation). - Updates expected harness outputs and adds a C++ change note entry.
File summaries
| File | Description |
|---|---|
| cpp/ql/lib/ext/bal.model.yml | Adds summaryModel rows for balber/baljsn/balxml encode/decode taint propagation. |
| cpp/ql/test/library-tests/dataflow/external-models/bal.cpp | New stub + tests exercising the added summaries (including round-trip). |
| cpp/ql/test/library-tests/dataflow/external-models/steps.expected | Updates expected step output for the new test coverage. |
| cpp/ql/test/library-tests/dataflow/external-models/flow.expected | Updates expected flow/model output to include the new summaries and resulting edges. |
| cpp/ql/lib/change-notes/2026-09-09-bal-codec-models.md | Records the new models as a minor analysis improvement. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c8ad9bd to
f3409d8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The PR description’s claim about int-only returns/no ReturnValue[*] rows conflicts with the added balxml models that introduce ReturnValue[*] flows for stream-returning overloads.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| # Decoding: stream -> object; the istream overloads also return the stream | ||
| - ["BloombergLP::balxml", "Decoder", true, "decode<TYPE>", "(istream &,TYPE *,const char *)", "", "Argument[*0]", "Argument[*1]", "taint", "manual"] | ||
| - ["BloombergLP::balxml", "Decoder", true, "decode<TYPE>", "(istream &,TYPE *,const char *)", "", "Argument[*0]", "ReturnValue[*]", "taint", "manual"] | ||
| - ["BloombergLP::balxml", "Decoder", true, "decode<TYPE>", "(streambuf *,TYPE *,const char *)", "", "Argument[*0]", "Argument[*1]", "taint", "manual"] | ||
| - ["BloombergLP::balxml", "Decoder", true, "decode<TYPE>", "(const char *,size_t,TYPE *,const char *)", "", "Argument[*0]", "Argument[*2]", "taint", "manual"] | ||
| - ["BloombergLP::balxml", "Decoder", true, "decodeAny<TYPE>", "(istream &,TYPE *,const char *)", "", "Argument[*0]", "Argument[*1]", "taint", "manual"] | ||
| - ["BloombergLP::balxml", "Decoder", true, "decodeAny<TYPE>", "(istream &,TYPE *,const char *)", "", "Argument[*0]", "ReturnValue[*]", "taint", "manual"] |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
This pull request looks good to me. I do have a few questions and suggestions for additional test sinks (if I'm understanding correctly).
| - ["BloombergLP::balxml", "Decoder", true, "decode<TYPE>", "(streambuf *,TYPE *,const char *)", "", "Argument[*0]", "Argument[*1]", "taint", "manual"] | ||
| - ["BloombergLP::balxml", "Decoder", true, "decode<TYPE>", "(const char *,size_t,TYPE *,const char *)", "", "Argument[*0]", "Argument[*2]", "taint", "manual"] | ||
| - ["BloombergLP::balxml", "Decoder", true, "decodeAny<TYPE>", "(istream &,TYPE *,const char *)", "", "Argument[*0]", "Argument[*1]", "taint", "manual"] | ||
| - ["BloombergLP::balxml", "Decoder", true, "decodeAny<TYPE>", "(istream &,TYPE *,const char *)", "", "Argument[*0]", "ReturnValue[*]", "taint", "manual"] |
There was a problem hiding this comment.
Am I understanding correctly that a few of the methods, rather than returning an int code, return a reference to the stream itself? So if we're encoding an object to the stream, both the object and the input stream parameter taint the stream return value. And if we're decoding from the stream to the object, only the stream parameter taints the stream return value.
There was a problem hiding this comment.
Yes, that's correct. Some of the overloads return reference to same stream passes to it as param and if input stream contains untrusted data it flows back to the return value as well. Thats why it uses the stream parameter as its taint source.
| // and balxml only accept bdlat sequence/choice types; the stubs do not enforce that, and | ||
| // most tests decode into a bsl::string so the result can be read back. The *_struct_* tests | ||
| // show that a decoded struct is tainted as a whole but not through its fields, because | ||
| // field accesses are not taint steps (see TaintTrackingUtil.qll). |
There was a problem hiding this comment.
There is a concept of TaintInheritingContent that can be used to make taint flow from an object to its fields. But it's intended for specific types of objects, not general flow from any object to its fields as we're talking about here.
There was a problem hiding this comment.
Thanks for clarifying. I have updated the comment to acknowledge TaintInheritingContent for selected fields. The documented limitation here concerns general object-to-field flow for arbitrary decoded types.
4305352 to
19da11a
Compare
Thanks @geoffw0 for feedback. I pushed new commit to address your feedbacks. |
Add flow summaries for the balber, baljsn and balxml codecs. decode and decodeAny move taint from the stream into the decoded object; encode and encodeAny move it from the object into the stream. balber and baljsn have a uniform argument layout across overloads and return an int status, so their rows carry no signature. balxml overloads differ in layout and two of them return the stream they were given, so each balxml row names its overload and the istream/ostream forms also get ReturnValue[*] rows. Not modelled: balxml::Decoder::decode(const char *filename, TYPE *), the two-step open() + decode(TYPE *) form, and the Formatter overloads of balxml::Encoder. Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
test(cpp): address bal codec review feedback Apply suggestion from @geoffw0 Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Encoding into a tainted stream does not taint the const input object. Remove the incorrect positive assertion, restoring agreement with the existing flow and step expectations.
58109f8 to
2c14c74
Compare
Adds flow summaries for the BDE bal package group codecs: balber (BER), baljsn (JSON), and balxml (XML). For each codec, decode is modeled as stream-to-object flow (Argument[*0] → Argument[*1]), and encode as object-to-stream flow (Argument[*1] → Argument[*0]).
These methods are the boundary between serialized data and application objects, so without these summaries, taint reaching a streambuf stops at the codec. Modeling encoding also preserves taint throughout the serialize/deserialize round trip. This continues the incremental BDE modeling from #22455 (bdlbb) and #22453 (bslx).
All six methods return an int status code, so there are no ReturnValue[] rows. Signatures are left empty so the streambuf, istream&/ostream&, and options-taking overloads all match.
Known limitations: