Conversation
There was a problem hiding this comment.
Pull request overview
Adds C++ taint-flow models for Bloomberg BDE bslx deserialization.
Changes:
- Models
ByteInStream,GenericInStream, andbdexStreamIn. - Covers scalar, string, array, reset, and fluent-call flows.
- Adds comprehensive external-model tests and release notes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
cpp/ql/lib/ext/bslx.model.yml |
Defines the new flow summaries. |
cpp/ql/test/library-tests/dataflow/external-models/bslx.cpp |
Exercises modeled APIs and chaining. |
cpp/ql/test/library-tests/dataflow/external-models/flow.expected |
Updates generated flow expectations. |
cpp/ql/test/library-tests/dataflow/external-models/steps.expected |
Updates generated summary-step expectations. |
cpp/ql/lib/change-notes/2026-08-27-bslx-models.md |
Documents the analysis improvement. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
77a4275 to
ad27014
Compare
Add flow summaries for the BDE byte-stream deserializers in BloombergLP::bslx: - ByteInStream and GenericInStream<STREAMBUF>: constructing (or, for ByteInStream, reset-ing) from a buffer/streambuf taints the stream, and every get* method propagates that taint into its output variable. get* returns *this, so a fluent row keeps chained calls tainted. - InStreamFunctions::bdexStreamIn: the generic BDEX entry point that populates any bdex-compatible object from a stream, modeled as stream -> object. Not a duplicate of the bsl.* container models or the std/bsl QL models, which cover only the bsl standard-library-alike types; the bslx BDE namespace had no coverage. Verified with a BloombergLP::bslx-shaped stub in the dataflow external-models harness.
ad27014 to
c7d50f6
Compare
Drop scalar getter output summaries while retaining fluent stream flow. Use a type-aware QL model for bdexStreamIn object outputs to exclude scalars. Add regression coverage for integer outputs, fluent chaining, strings, and user-defined objects, and regenerate external-model expectations. Consolidate the review fixes and retain the merged main history. Validation: all five external-model tests pass without --learn; QL formatting and git diff --check pass.
d889939 to
ccff432
Compare
jketema
left a comment
jketema
left a comment
There was a problem hiding this comment.
Some comments. As before I've tried make some of the text shorter.
| BloombergLP::bsls::Types::Uint64 ullbuf[16]; | ||
| float fbuf[16]; | ||
| double dbuf[16]; | ||
| stream.getLength(i).getVersion(i).getInt8(c).getUint8(uc).getInt16(s).getUint16(us).getInt24(i).getUint24(ui).getInt32(i).getUint32(ui).getInt40(ll).getUint40(ull).getInt48(ll).getUint48(ull).getInt56(ll).getUint56(ull).getInt64(ll).getUint64(ull).getFloat32(f).getFloat64(d).getString(str).getArrayInt8(cbuf, 16).getArrayUint8(ucbuf, 16).getArrayInt16(sbuf, 16).getArrayUint16(usbuf, 16).getArrayInt24(ibuf, 16).getArrayUint24(uibuf, 16).getArrayInt32(ibuf, 16).getArrayUint32(uibuf, 16).getArrayInt40(llbuf, 16).getArrayUint40(ullbuf, 16).getArrayInt48(llbuf, 16).getArrayUint48(ullbuf, 16).getArrayInt56(llbuf, 16).getArrayUint56(ullbuf, 16).getArrayInt64(llbuf, 16).getArrayUint64(ullbuf, 16).getArrayFloat32(fbuf, 16).getArrayFloat64(dbuf, 16); |
There was a problem hiding this comment.
Ideally we should have a source and a sink call, so we can have a // $ ir annotation. There's no need to test this separately per function call.
There was a problem hiding this comment.
Added a source and sink to both coverage tests. Each getter chain now ends with getString into a fresh output and a // $ ir sink assertion, verifying flow through the full chain.
| BloombergLP::bsls::Types::Uint64 ullbuf[16]; | ||
| float fbuf[16]; | ||
| double dbuf[16]; | ||
| stream.getLength(i).getVersion(i).getInt8(c).getUint8(uc).getInt16(s).getUint16(us).getInt24(i).getUint24(ui).getInt32(i).getUint32(ui).getInt40(ll).getUint40(ull).getInt48(ll).getUint48(ull).getInt56(ll).getUint56(ull).getInt64(ll).getUint64(ull).getFloat32(f).getFloat64(d).getString(str).getArrayInt8(cbuf, 16).getArrayUint8(ucbuf, 16).getArrayInt16(sbuf, 16).getArrayUint16(usbuf, 16).getArrayInt24(ibuf, 16).getArrayUint24(uibuf, 16).getArrayInt32(ibuf, 16).getArrayUint32(uibuf, 16).getArrayInt40(llbuf, 16).getArrayUint40(ullbuf, 16).getArrayInt48(llbuf, 16).getArrayUint48(ullbuf, 16).getArrayInt56(llbuf, 16).getArrayUint56(ullbuf, 16).getArrayInt64(llbuf, 16).getArrayUint64(ullbuf, 16).getArrayFloat32(fbuf, 16).getArrayFloat64(dbuf, 16); |
There was a problem hiding this comment.
Ideally we should have a source and a sink call, so we can have a // $ ir annotation. There's no need to test this separately per function call.
| private class BdexStreamIn extends TaintFunction { | ||
| BdexStreamIn() { | ||
| this.hasQualifiedName("BloombergLP::bslx::InStreamFunctions", "bdexStreamIn") and | ||
| this.getParameter(1).getUnspecifiedType().(ReferenceType).getBaseType().getUnspecifiedType() | ||
| instanceof Class | ||
| } | ||
|
|
||
| override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { | ||
| input.isParameterDeref(0) and | ||
| output.isParameterDeref(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
I prefer to more generic version in the yml file you had before, even though it does allow flow through scalar values. There is no particular harm in that flow. The difference with the other cases is that no explicit modelling is required here that bloats the yml file. In this case I would just trust the sanitizers in the queries to do their job, and if this causes performance problems somewhere down the line, then the better approach would be to extend the yml format so we can accurately model templated functions like these.
There was a problem hiding this comment.
Restored the generic bdexStreamIn yaml summary and updated the test to allow scalar flow through bdexStreamIn.
Add flow summaries for the BDE byte-stream deserializers in BloombergLP::bslx:
Not a duplicate of the bsl.* container models or the std/bsl QL models, which cover only the bsl standard-library-alike types; the bslx BDE namespace had no coverage.