Thread: [RFC] Clang plugin for catching suspicious typedef casting

[RFC] Clang plugin for catching suspicious typedef casting

From
Dmitry Dolgov
Date:
Hi,

In the commit [1] one side change was to fix mixed up usage of
BlockNumber and Buffer variables. This was a one-off manual effort, and
indeed it hardly seems possible to get compilation warnings for such
code, as long as the underlying types could be converted in a standard
conforming way. As far as I see such conversions are acceptable and
used every now and then in the project, but they may hide some subtle
issues.

One interesting way I found to address this was to benefit from clang
plugin system [2]. A clang plugin allows you to run some frontend
actions when compiling code with clang, and this approach is used in
some large open source projects (e.g. I've got my inspiration largely
from libreoffice [3]). After a little experimenting I could teach such a
plugin to warn about situations similar to the one described above. What
it does is:

* It visits all the function call expressions
* Verify if the function arguments are defined via typedef
* Compare the actual argument with the function declaration
* Consult with the suppression rules to minimize false positives

In the end the plugin catches the error mentioned above, and we get
something like this:

    ginget.c:143:41: warning: Typedef check: Expected 'BlockNumber' (aka 'unsigned int'),
            got 'Buffer' (aka 'int') in PredicateLockPage PredicateLockPage(btree->index, stack->buffer, snapshot);

Of course the plugin produces more warning, and I haven't checked all of
them yet -- some probably would have to be ignored as well. But in the
long run I'm pretty confident it's possible to fine tune this logic and
suppression rules to get minimum noise.

As I see it, there are advantages of using plugins in such way:

* Helps automatically detect some issues
* Other type of plugins could be useful to support large undertakings,
  where a lot of code transformation is involved.

And of course disadvantages as well:

* It has to be fine-tuned to be useful
* It's compiler dependent, not clear how to always exercise it

I would like to get your opinion, folks. Does it sound interesting
enough for the community, is it worth it to pursue the idea?

Some final notes about infrastructure bits. Never mind cmake in there --
it was just for a quick start, I'm going to convert it to something
else. There are some changes needed to tell the compiler to actually
load the plugin, those of course could be done much better as well. I
didn't do anything with meson here, because it turns out meson tends to
enclose the plugin file with '-Wl,--start-group ... -Wl,--end-group' and
it breaks the plugin discovery.

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=126552c85c1cfb6ce6445159b8024cfa5631f33e
[2]: https://clang.llvm.org/docs/ClangPlugins.html
[3]: https://git.libreoffice.org/core/+/refs/heads/master/compilerplugins/clang/

Attachment

Re: [RFC] Clang plugin for catching suspicious typedef casting

From
"Tristan Partin"
Date:
This is the first I am learning about clang plugins. Interesting
concept. Did you give any thought to using libclang instead of
a compiler plugin? I am kind of doing similar work, but I went with
libclang instead of a plugin.

--
Tristan Partin
Neon (https://neon.tech)



Re: [RFC] Clang plugin for catching suspicious typedef casting

From
Dmitry Dolgov
Date:
> On Thu, Aug 03, 2023 at 12:23:52PM -0500, Tristan Partin wrote:
>
> This is the first I am learning about clang plugins. Interesting concept.
> Did you give any thought to using libclang instead of a compiler plugin? I
> am kind of doing similar work, but I went with libclang instead of a plugin.

Nope, never thought about trying libclang. From the clang documentation
it seems a plugin is a suitable interface if one wants to:

    special lint-style warnings or errors for your project

Which is what I was trying to achieve. Are there any advantages of
libclang that you see?



Re: [RFC] Clang plugin for catching suspicious typedef casting

From
Peter Eisentraut
Date:
On 03.08.23 18:56, Dmitry Dolgov wrote:
> I would like to get your opinion, folks. Does it sound interesting
> enough for the community, is it worth it to pursue the idea?

I think it's interesting, and doesn't look too invasive.

Maybe we can come up with three or four interesting use cases and try to 
implement them.  BlockNumber vs. Buffer type checking is obviously a bit 
marginal to get anyone super-excited, but it's a reasonable demo.

Also, how stable is the plugin API?  Would we need to keep updating 
these plugins for each new clang version?



Re: [RFC] Clang plugin for catching suspicious typedef casting

From
Dmitry Dolgov
Date:
> On Wed, Aug 09, 2023 at 03:23:32PM +0200, Peter Eisentraut wrote:
> On 03.08.23 18:56, Dmitry Dolgov wrote:
> > I would like to get your opinion, folks. Does it sound interesting
> > enough for the community, is it worth it to pursue the idea?
>
> I think it's interesting, and doesn't look too invasive.
>
> Maybe we can come up with three or four interesting use cases and try to
> implement them.  BlockNumber vs. Buffer type checking is obviously a bit
> marginal to get anyone super-excited, but it's a reasonable demo.
>
> Also, how stable is the plugin API?  Would we need to keep updating these
> plugins for each new clang version?

From the first glance the API itself seems to be stable -- I didn't find
many breaking changes for plugins in the release notes over the last
five releases. The git history for such definitions as ASTConsumer or
RecursiveASTVisitor also doesn't seem to be very convoluted.

But libreoffice example shows, that some updating would be necessary --
they've got about a dozen of places in the code that depend on the clang
version. From what I see it's usually not directly related to the plugin
API, and looks more like our opaque pointers issue.



Re: [RFC] Clang plugin for catching suspicious typedef casting

From
Xing Guo
Date:
Hi,

On 8/10/23, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>> On Wed, Aug 09, 2023 at 03:23:32PM +0200, Peter Eisentraut wrote:
>> On 03.08.23 18:56, Dmitry Dolgov wrote:
>> > I would like to get your opinion, folks. Does it sound interesting
>> > enough for the community, is it worth it to pursue the idea?
>>
>> I think it's interesting, and doesn't look too invasive.

+1.

>> Maybe we can come up with three or four interesting use cases and try to
>> implement them.  BlockNumber vs. Buffer type checking is obviously a bit
>> marginal to get anyone super-excited, but it's a reasonable demo.

Several months ago, I implemented a clang plugin[1] for checking
suspicious return/goto/break/continue in PG_TRY/PG_CATCH blocks and it
found unsafe codes in Postgres[2]. It's implemented using clang's AST
matcher API.

I would like to contribute it well if this RFC can be accepted.

[1] https://github.com/higuoxing/clang-plugins/blob/main/lib/ReturnInPgTryBlockChecker.cpp
[2] https://www.postgresql.org/message-id/flat/CACpMh%2BCMsGMRKFzFMm3bYTzQmMU5nfEEoEDU2apJcc4hid36AQ%40mail.gmail.com

-- 
Best Regards,
Xing



Re: [RFC] Clang plugin for catching suspicious typedef casting

From
"Tristan Partin"
Date:
On Fri Aug 4, 2023 at 5:47 AM CDT, Dmitry Dolgov wrote:
> > On Thu, Aug 03, 2023 at 12:23:52PM -0500, Tristan Partin wrote:
> >
> > This is the first I am learning about clang plugins. Interesting concept.
> > Did you give any thought to using libclang instead of a compiler plugin? I
> > am kind of doing similar work, but I went with libclang instead of a plugin.
>
> Nope, never thought about trying libclang. From the clang documentation
> it seems a plugin is a suitable interface if one wants to:
>
>     special lint-style warnings or errors for your project
>
> Which is what I was trying to achieve. Are there any advantages of
> libclang that you see?

Only advantage I see to using libclang is that you can run programs
built with libclang regardless of what your C compiler is. I typically
use GCC.

I think your idea of automating this kind of thing is great no matter
how it is implemented.

--
Tristan Partin
Neon (https://neon.tech)



Re: [RFC] Clang plugin for catching suspicious typedef casting

From
Dmitry Dolgov
Date:
> On Fri, Dec 01, 2023 at 04:01:05PM -0600, Tristan Partin wrote:
> On Fri Aug 4, 2023 at 5:47 AM CDT, Dmitry Dolgov wrote:
> > > On Thu, Aug 03, 2023 at 12:23:52PM -0500, Tristan Partin wrote:
> > >
> > > This is the first I am learning about clang plugins. Interesting concept.
> > > Did you give any thought to using libclang instead of a compiler plugin? I
> > > am kind of doing similar work, but I went with libclang instead of a plugin.
> >
> > Nope, never thought about trying libclang. From the clang documentation
> > it seems a plugin is a suitable interface if one wants to:
> >
> >     special lint-style warnings or errors for your project
> >
> > Which is what I was trying to achieve. Are there any advantages of
> > libclang that you see?
>
> Only advantage I see to using libclang is that you can run programs built
> with libclang regardless of what your C compiler is. I typically use GCC.
>
> I think your idea of automating this kind of thing is great no matter how it
> is implemented.

Yeah, absolutely.

Sorry, haven't had a chance to follow up on the patch, but I'm planing
to do so. I guess the important part, as Peter mentioned above in the
thread, is to figure out more use cases which could be usefully
augmented with such compile time checks. At the moment I don't have any
other except BlockNumber vs Buffer, so I'm going to search through the
hackers looking for more potential targets. If anyone got a great idea
right away about where compile-time checks could be useful, feel free to
share!



Re: [RFC] Clang plugin for catching suspicious typedef casting

From
Peter Geoghegan
Date:
On Sun, Dec 3, 2023 at 6:31 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> > Only advantage I see to using libclang is that you can run programs built
> > with libclang regardless of what your C compiler is. I typically use GCC.
> >
> > I think your idea of automating this kind of thing is great no matter how it
> > is implemented.
>
> Yeah, absolutely.

What is the difference between a clang plugin (or whatever this is),
and a custom clang-tidy check? Are they the same thing?

I myself use clang-tidy through clangd. It has a huge number of
checks, plus some custom checks that are only used by certain open
source projects.

An example of a check that seems like it would be interesting to
Postgres hackers:

https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone/signal-handler.html

An example of a custom clang-tidy check, used for the Linux Kernel:

https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/linuxkernel/must-use-errs.html

Your idea of starting with a check that identifies when BlockNumber
and Buffer variables were confused seems like the right general idea
to me. It's just about impossible for this to be a false positive in practice,
which is important. But, I wonder, is there a way of accomplishing the
same thing without any custom code? Seems like the general idea of not
confusing two types like BlockNumber and Buffer might be a generic
one?

Some random ideas in this area:

* A custom check that enforces coding standards within signal handlers
-- the generic one that I linked to might need to be customized, in
whatever way (don't use it myself).

* A custom check that enforces a coding standard that applies within
critical sections.

We already have an assertion that catches memory allocations made
within a critical section. It might make sense to have tooling that
can detect other kinds of definitely-unsafe things in critical
sections. For example, maybe it would be possible to detect when
XLogRegisterBufData() has been passed a pointer to something on the
stack that will go out of scope, in a way that leaves open the
possibility of reading random stuff from the stack once inside
XLogInsert(). AddressSanitizer's use-after-scope can detect that sort
of thing, though not reliably.

Not at all sure about how feasible any of this is...


--
Peter Geoghegan



Re: [RFC] Clang plugin for catching suspicious typedef casting

From
Dmitry Dolgov
Date:
> On Sun, Dec 03, 2023 at 07:02:55PM -0800, Peter Geoghegan wrote:
> On Sun, Dec 3, 2023 at 6:31 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> > > Only advantage I see to using libclang is that you can run programs built
> > > with libclang regardless of what your C compiler is. I typically use GCC.
> > >
> > > I think your idea of automating this kind of thing is great no matter how it
> > > is implemented.
> >
> > Yeah, absolutely.
>
> What is the difference between a clang plugin (or whatever this is),
> and a custom clang-tidy check? Are they the same thing?

Good question. Obviously one difference is that a clang plugin is more
tightly integrated into the build process, where a custom clang-tidy
check could be used independently. IIUC they also use different API, AST
Consumer vs AST Matchers, but so far I have no idea what consequences
does it have.

Clang tooling page has this to say about choosing the right interface
[1], where clang-tidy seems to fall into LibTooling cathegory:

    Clang Plugins allow you to run additional actions on the AST as part
    of a compilation. Plugins are dynamic libraries that are loaded at
    runtime by the compiler, and they’re easy to integrate into your
    build environment.

    Canonical examples of when to use Clang Plugins:
    * special lint-style warnings or errors for your project creating
    * additional build artifacts from a single compile step

    [...]

    LibTooling is a C++ interface aimed at writing standalone tools, as
    well as integrating into services that run clang tools. Canonical
    examples of when to use LibTooling:

    * a simple syntax checker
    * refactoring tools

> I myself use clang-tidy through clangd. It has a huge number of
> checks, plus some custom checks that are only used by certain open
> source projects.
>
> An example of a check that seems like it would be interesting to
> Postgres hackers:
>
> https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone/signal-handler.html
>
> An example of a custom clang-tidy check, used for the Linux Kernel:
>
> https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/linuxkernel/must-use-errs.html

Yeah, those look interesting, and might be even worth including
independently of the topic in this thread.

> Your idea of starting with a check that identifies when BlockNumber
> and Buffer variables were confused seems like the right general idea
> to me. It's just about impossible for this to be a false positive in practice,
> which is important. But, I wonder, is there a way of accomplishing the
> same thing without any custom code? Seems like the general idea of not
> confusing two types like BlockNumber and Buffer might be a generic
> one?

Agree, the example in this patch could be generalized.

> * A custom check that enforces coding standards within signal handlers
> -- the generic one that I linked to might need to be customized, in
> whatever way (don't use it myself).
>
> * A custom check that enforces a coding standard that applies within
> critical sections.
>
> We already have an assertion that catches memory allocations made
> within a critical section. It might make sense to have tooling that
> can detect other kinds of definitely-unsafe things in critical
> sections. For example, maybe it would be possible to detect when
> XLogRegisterBufData() has been passed a pointer to something on the
> stack that will go out of scope, in a way that leaves open the
> possibility of reading random stuff from the stack once inside
> XLogInsert(). AddressSanitizer's use-after-scope can detect that sort
> of thing, though not reliably.

Interesting ideas, thanks for sharing. I'm going to try implementing
some of those.

[1]: https://clang.llvm.org/docs/Tooling.html