bkryza / clang-uml

Customizable automatic UML diagram generator for C++ based on Clang.
Apache License 2.0
592 stars 40 forks source link

fix segfault in name for unnamed undeclared types #277

Closed pogobanane closed 3 months ago

pogobanane commented 3 months ago

type->getAsTagDecl() may return null resulting in a segfault. E.g. in the following example:

struct Foo {
  struct {
    uint64_t len;
    uint64_t flags;
  } __attribute__((packed)) bars[BARS]; <-- the type of this bars
  ...
};

I found this segfault while working on this code https://github.com/vmuxIO/vmuxIO/commit/d1d44aa84158a6bc2955e62f597b6794b66f221f (just uml). This patch solves that segfault.

I have no prior experience with clang, but i hope my solution is still valid.

bkryza commented 3 months ago

@pogobanane Thanks for the PR. I've managed to reproduce the problem however the cause of the issue is slightly different. Anonymous structs and unions are supported and covered by a separated test case: https://clang-uml.github.io/md_docs_2test__cases_2t00037.html, but what I forgot to test for are arrays of anonymous structs :-)

If you'd like to take a go at fixing it, please do the following:

  1. Extend the test case code here by adding your array of bars structs in the ST class.
  2. Add checks in here for existence of class ST::(bars) and for an aggregation relationship from ST to ST::(bars)
  3. At the top of the to_string method you've modified in your PR add the following code:
       if (type->isArrayType()) {
        return fmt::format("{}[]",
            to_string(type->getAsArrayTypeUnsafe()->getElementType(), ctx,
                try_canonical));
       }
  4. Extend this if-else block with a case for array type
  5. You can keep your check for nullptr in getTagDecl for additional safety but please change NULL to nullptr.

If you don't have time to do this let me know - I'll fix it today.

Thanks again!

pogobanane commented 3 months ago

@bkryza I won't have enough time the next few days to complete your suggestions. I'm lacking context knowledge which is why i barely understand your proposed changes.

bkryza commented 3 months ago

@pogobanane No problem. I've merged your PR so that you at least get credit for reporting the issue. I've also added the relevant fixes in the master branch so now if you run in it on your code you should get that anonymous struct as well as a relationship to it in the diagram.

pogobanane commented 3 months ago

Works now. Thanks!

pogobanane commented 3 months ago

Nitpicky, and i dont really mind, but i noticed the following: Since isArrayType now matches all arrays (not only unnamed ones), it slightly changes the to_string behaviour of fixed length arrays:

-        +txFrame : char[9000]
+        +txFrame : char[]
bkryza commented 3 months ago

@pogobanane Thanks for spotting this. I've just committed fix for this to master as well as few other related fixes. I've also extended the test case for this: https://github.com/bkryza/clang-uml/blob/master/docs/test_cases/t00037.md