Page MenuHomePhabricator

clang-tidy: new bugprone-pointer-cast-widening
Changes PlannedPublic

Authored by jankratochvil on Dec 19 2019, 7:31 AM.

Details

Summary

Check for cast of a pointer to wider (even unsigned) integer. This will sign-extend the pointer which happens on 32-bit hosts for 64-bit integers:

void *p=(void *)0x80000000;
printf(  "%p"        "\n",                        p ); // 0x80000000
printf("0x%" PRIxPTR "\n",             (uintptr_t)p ); // 0x80000000
printf("0x%" PRIx64  "\n",
                       reinterpret_cast<uint64_t>(p)); // 0xffffffff80000000
printf("0x%" PRIx64  "\n",             (uint64_t) p ); // 0xffffffff80000000
printf("0x%" PRIx64  "\n",   (uint64_t)(uintptr_t)p ); // 0x80000000

It has caught a bug on ARM32: D71498 (and D71514)

Diff Detail

Event Timeline

jankratochvil created this revision.Dec 19 2019, 7:31 AM

Example output:

PATH="$PWD/bin:$PATH" ~/redhat/llvm-monorepo/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py -checks='-*,bugprone-pointer-cast-widening' /home/jkratoch/redhat/llvm-monorepo/lldb/source/

Enabled checks:
    bugprone-pointer-cast-widening

/home/jkratoch/redhat/llvm-monorepo/lldb/source/Core/PluginManager.cpp:89:35: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
  return reinterpret_cast<FPtrTy>(reinterpret_cast<intptr_t>(VPtr));
                                  ^
 - What is the use of intptr_t?
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRMemoryMap.cpp:333:29: warning: do not use cast of a pointer 'std::__shared_ptr<lldb_private::Process, __gnu_cxx::_S_atomic>::element_type *' (aka 'lldb_private::Process *') to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
              __FUNCTION__, (lldb::addr_t)process_sp.get(),
                            ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRMemoryMap.cpp:582:42: warning: do not use cast of a pointer 'const uint8_t *' (aka 'const unsigned char *') to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
              (uint64_t)process_address, (uint64_t)bytes, (uint64_t)size,
                                         ^
 - A bug, not yet submitted.
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRMemoryMap.cpp:713:42: warning: do not use cast of a pointer 'uint8_t *' (aka 'unsigned char *') to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
              (uint64_t)process_address, (uint64_t)bytes, (uint64_t)size,
                                         ^
 - A bug, not yet submitted.
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Host/common/HostInfoBase.cpp:252:32: warning: do not use cast of a pointer 'bool (*)(lldb_private::FileSpec &)' to non-uintptr_t 'intptr_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
      reinterpret_cast<void *>(reinterpret_cast<intptr_t>(
                               ^
 - What is the use of intptr_t?
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Host/posix/DomainSocket.cpp:48:20: warning: do not use cast of a pointer 'char *' to non-uintptr_t 'size_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
    saddr_un_len = SUN_LEN(saddr_un);
                   ^
/usr/include/sys/un.h:40:24: note: expanded from macro 'SUN_LEN'
# define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)        \
                       ^
 - False positive?
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Expression/IRExecutionUnit.cpp:351:53: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
        function.getName().str().c_str(), external, (lldb::addr_t)fun_ptr));
                                                    ^
 - A bug, submitted as: D71498
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp:358:24: warning: do not use cast of a pointer 'clang::NamedDecl *' to non-uintptr_t 'uint64_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
                       reinterpret_cast<uint64_t>(result_decl), false);
                       ^
 - A bug, not yet submitted.
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:139:23: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
    AppendBounds(str, reinterpret_cast<lldb::addr_t>(info.si_lower),
                      ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:140:18: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
                 reinterpret_cast<lldb::addr_t>(info.si_upper),
                 ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:141:18: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
                 reinterpret_cast<lldb::addr_t>(info.si_addr));
                 ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Plugins/Process/POSIX/CrashReason.cpp:147:31: warning: do not use cast of a pointer 'void *' to non-uintptr_t 'lldb::addr_t' (aka 'unsigned long') which may sign-extend [bugprone-pointer-cast-widening]
                              reinterpret_cast<lldb::addr_t>(info.si_addr));
                              ^
 - A bug, submitted as: D71514
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Utility/Stream.cpp:159:65: warning: do not use cast of a pointer 'const void *' to non-uintptr_t 'ptrdiff_t' (aka 'long') which may sign-extend [bugprone-pointer-cast-widening]
  Printf("0x%.*tx", static_cast<int>(sizeof(const void *)) * 2, (ptrdiff_t)p);
                                                                ^
 - False positive?

Output filtered by my script.

labath resigned from this revision.Dec 19 2019, 7:58 AM

Though this *was* my idea, I don't really feel qualified to review code here.

However, some things to consider:

  • disallowing casts to intptr_t seems too restrictive -- I doubt many people are doing that, but I guess this type exists for a reason, and since the type (and it's signedness) is spelled out in the source, it shouldn't be too surprising that sign-extension can happen later
  • requiring a literal uintptr_t (or a typedef of it) may be also problematic -- the user could obtain an integer type of the same bit width through some other means (e.g. #ifdef). OTOH, without that (and just checking the bit width for instance), one would have to actually compile for a 32-bit target to get this warning. I don't know what's the practice for this in clang-tidy...

Please add documentation and mention new check in Release Notes.

Eugene.Zelenko edited reviewers, added: hokein, aaron.ballman; removed: labath.Dec 19 2019, 9:08 AM
Eugene.Zelenko removed a subscriber: Restricted Project.
  • disallowing casts to intptr_t seems too restrictive -- I doubt many people are doing that, but I guess this type exists for a reason, and since the type (and it's signedness) is spelled out in the source, it shouldn't be too surprising that sign-extension can happen later

I was trying to find what is intptr_t good for and I haven't found any valid reason. It seems to me nobody knows that either. Which is why I find correct to report it. This checker has many false positives (or "not really a bug") anyway.

  • requiring a literal uintptr_t (or a typedef of it) may be also problematic -- the user could obtain an integer type of the same bit width through some other means (e.g. #ifdef). OTOH, without that (and just checking the bit width for instance), one would have to actually compile for a 32-bit target to get this warning. I don't know what's the practice for this in clang-tidy...

Yes, I wanted first to check the widths but then I realized user would need a 32-bit host for that which is too difficult to (1) get nowadays and (2) primarily to build there LLVM.

jankratochvil planned changes to this revision.Dec 19 2019, 9:19 AM

Wrote the documentation and Release Notes entry, thanks for the review.

Eugene.Zelenko added inline comments.Dec 19 2019, 1:25 PM
clang-tools-extra/docs/ReleaseNotes.rst
112

Checks. One sentence should be enough.

clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
6

void is unnecessary.

jankratochvil marked 3 inline comments as done.
jankratochvil added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
112

Not sure if the wording is OK in such a long sentence (non-native English here).

jankratochvil marked an inline comment as done and an inline comment as not done.Dec 19 2019, 2:31 PM
  • disallowing casts to intptr_t seems too restrictive -- I doubt many people are doing that, but I guess this type exists for a reason, and since the type (and it's signedness) is spelled out in the source, it shouldn't be too surprising that sign-extension can happen later

I was trying to find what is intptr_t good for and I haven't found any valid reason. It seems to me nobody knows that either. Which is why I find correct to report it. This checker has many false positives (or "not really a bug") anyway.

I agree that restricting casts to intptr_t is too restrictive. intptr_t is good for all the same things as uintptr_t -- it depends on why you want to use the pointer value as an integer value as to what it is good for.

I'm also concerned about the number of false positives found by this checker, but I think fixing this would fix some of the more egregious false positives.

  • requiring a literal uintptr_t (or a typedef of it) may be also problematic -- the user could obtain an integer type of the same bit width through some other means (e.g. #ifdef). OTOH, without that (and just checking the bit width for instance), one would have to actually compile for a 32-bit target to get this warning. I don't know what's the practice for this in clang-tidy...

Yes, I wanted first to check the widths but then I realized user would need a 32-bit host for that which is too difficult to (1) get nowadays and (2) primarily to build there LLVM.

Have you considered language extensions like __ptr32, __ptr64, __sptr, and __uptr (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we also support? I think those probably should factor into this new check as well.

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp
22–23

Why limiting this to C++ and >= C99? You can get pointer widening casts through extensions in C89, for instance.

25

I think you should use hasCastKind() in this matcher as well.

37

How well does this work with something like std::uinptr_t from <cstdint>?

clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
4

You should replicate the contents of the system header rather than including it for tests -- this ensures you are testing the same thing on all platforms.

12

This should not diagnose.

18

I'd also like to see tests for implicit casts as well as pointer width and sign extension language extensions.

jankratochvil planned changes to this revision.Jan 5 2020, 2:33 AM
jankratochvil marked 8 inline comments as done.Jan 15 2020, 7:53 AM

I agree that restricting casts to intptr_t is too restrictive.

Permitted intptr_t. But then implemented also checking a cast of intptr_t to wider integral types.

Have you considered language extensions like __ptr32, __ptr64, __sptr, and __uptr (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we also support?

I think only __uptr support would matter to prevent some false positives. I am not sure it is used anywhere. Should I really implement it?

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp
22–23

Removed the check completely.

37

It works fine because its

using::uintptr_t;

is recogized as a typedef in the loop above from namespace-less `uintptr_t'. Sure thanks for notifying me to check that.

clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
18

Done implicit cases. But I do not understand what you mean by the rest - could you provide an example? Thanks.

Have you considered language extensions like __ptr32, __ptr64, __sptr, and __uptr (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we also support?

I think only __uptr support would matter to prevent some false positives. I am not sure it is used anywhere. Should I really implement it?

It's not super common so if it's a problem, you can punt on it. It is used within the Win32 SDK though (I found at least a few uses of it). If you don't want to add support for them right now, you can just throw a FIXME into the code so we remember to add support for it at some point.

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.cpp
32

str should be Str.

46

static llvm::StringSet<> IntPtrs{"uintptr_t", "intptr_t"};

67

Similar here as above.

clang-tools-extra/clang-tidy/bugprone/PointerCastWideningCheck.h
28

This function doesn't need to be a member function, it doesn't rely on the class state whatsoever. I think the function should not take the container but instead be a templated function accepting iterators.

28–31

These should be findType, checkPointer, and checkIntegral by our usual naming conventions. Also, strset should be StrSet per conventions (same in the definition).

clang-tools-extra/docs/clang-tidy/checks/bugprone-pointer-cast-widening.rst
24

This seems outdated?

clang-tools-extra/test/clang-tidy/checkers/bugprone-pointer-cast-widening.cpp
18

I was talking about test cases involving __ptr32 or __uptr, if we wanted to support them.

21

Remove commented out code.

24

Remove commented out code.

32

Can you also show that C-style casts to intptr_t are not diagnosed? e.g., intptr_t ip2 = (inptr_t)p;

37

Remove commented-out code.

jankratochvil planned changes to this revision.Jan 16 2020, 9:31 AM
jankratochvil marked 2 inline comments as done.