Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++: Support for extension models (.yml) #16665

Merged
merged 15 commits into from
Jun 6, 2024
Merged

C++: Support for extension models (.yml) #16665

merged 15 commits into from
Jun 6, 2024

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jun 4, 2024

Add support for extension models in C/C++, that is, defining source, sink and summary models in .yml files rather than by writing QL classes to contain them.

In addition to the changes, tests and documentation for this feature, I have added a partial model for the Boost.Asio network library (using extension models). This was necessary in order to create verified examples for the documentation. In addition, it adds a further layer of testing for the feature and a few new models for CPP.

@geoffw0 geoffw0 added the C++ label Jun 4, 2024
@geoffw0 geoffw0 requested a review from a team as a code owner June 4, 2024 10:04
@geoffw0 geoffw0 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jun 4, 2024
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good to me, but I do have a couple of questions!

cpp/ql/lib/ext/Boost.Asio.model.yml Outdated Show resolved Hide resolved
The sixth value should be left empty and is out of scope for this documentation.
The remaining values are used to define the ``access path``, the ``kind``, and the ``provenance`` (origin) of the source.

- The seventh value ``"Argument[*1]"`` is the ``access path``, which means that the sink is the first indirection (or pointed-to value, ``*``) of the second argument (``Argument[1]``) passed to the function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do other languages also call this "access path"? This is not really aligned with what an "access path" in the dataflow library is (or the way we talk about access paths internally). I'd much rather we called this "input specification" and "input specification" or something like that, but if other languages are already calling this "access paths" I guess we have to live with it 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was described as an "access path" in the csharp doc this is based on. I'm happy to rephrase if "input specification" is preferred?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do prefer "input specification", but I suppose there's some benefits to language consistency 🤔 I'm happy with whatever you decide on this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Please could you check you're happy with the new phrasing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question, will there be specifically C/C++ sources/sinks (mainly sinks) that other languages may not have? Something like custom allocation and deallocation functions in a custom library versus using malloc and free.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could definitely add a MaD entry point for defining custom allocation and deallocation functions. This should be fairly straightforward to do already (although the MaD rows will need to be defined in .qll files until this PR has been merged. After that, they can go into .yml files)

@saritai saritai assigned saritai and unassigned saritai Jun 5, 2024
subatoi
subatoi previously approved these changes Jun 5, 2024
Copy link
Contributor

@subatoi subatoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation-wise this looks good: I can see it's very similar to the others (eg C#) 👍

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 6, 2024

I should say that the DCA run LGTM. keepassxc analysis was 13.8% slower, but that may well just be a wobbly result.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MathiasVP MathiasVP merged commit 5deb900 into github:main Jun 6, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ documentation ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants