Bug 80963 - UBSAN false positive with visibility=hidden
Summary: UBSAN false positive with visibility=hidden
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: sanitizer (show other bugs)
Version: 7.1.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: visibility
Depends on:
Blocks: visibility
  Show dependency treegraph
 
Reported: 2017-06-03 15:09 UTC by Jan Engelhardt
Modified: 2021-09-17 00:10 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Engelhardt 2017-06-03 15:09:24 UTC
$ cat lib.h
struct Archive {
        virtual void foo() = 0;
};
__attribute__((visibility("default"))) Archive *factory();

$ cat libimpl.cpp
#include "lib.h"
struct ArchiveImpl : Archive { void foo(); };
void ArchiveImpl::foo() {}
Archive *factory() { return new ArchiveImpl; }

$ cat main.cpp 
#include "lib.h"
int main(void) {
        factory()->foo();
}

$ make
g++ -fPIC -o libimpl.so -shared libimpl.cpp -fvisibility=hidden -Wall -fsanitize=undefined -lubsan
g++ -o main main.cpp ./libimpl.so -fvisibility=hidden -Wall -fsanitize=undefined -lubsan

$ ./main
main.cpp:3:16: runtime error: member call on address 0x000000dcfc20 which does not point to an object of type 'Archive'
0x000000dcfc20: note: object is of type 'ArchiveImpl'
 00 00 00 00  88 ed 59 dc 84 7f 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  21 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'ArchiveImpl'

The symbol table of main or libimpl.so do not appear to change when removing/adding -fvisiblity=hidden (no added/removed symbols, just address changes), so I wonder what exactly it is that UBSAN is trying to look up and not finding.

$ g++ -v
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib64/gcc/x86_64-suse-linux/7/lto-wrapper
OFFLOAD_TARGET_NAMES=hsa:nvptx-none
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info --mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64 --enable-languages=c,c++,objc,fortran,obj-c++,ada,go --enable-offload-targets=hsa,nvptx-none=/usr/nvptx-none, --without-cuda-driver --enable-checking=release --disable-werror --with-gxx-include-dir=/usr/include/c++/7 --enable-ssp --disable-libssp --disable-libvtv --disable-libcc1 --enable-plugin --with-bugurl=http://bugs.opensuse.org/ --with-pkgversion='SUSE Linux' --with-slibdir=/lib64 --with-system-zlib --enable-__cxa_atexit --enable-libstdcxx-allocator=new --disable-libstdcxx-pch --enable-version-specific-runtime-libs --with-gcc-major-version-only --enable-linker-build-id --enable-linux-futex --enable-gnu-indirect-function --program-suffix=-7 --without-system-libunwind --enable-multilib --with-arch-32=x86-64 --with-tune=generic --build=x86_64-suse-linux --host=x86_64-suse-linux
Thread model: posix
gcc version 7.1.1 20170530 [gcc-7-branch revision 248621] (SUSE Linux)
Comment 1 Andrew Pinski 2017-06-03 19:28:33 UTC
I think the error message is correct as the class has a linkage of hidden in the shared library.

That is Archive in the shared library and in the main executable are considered two different classes.

To fix this you need to have the visibility default attribute on the class Archive .
Comment 2 Andrew Pinski 2017-06-03 19:28:53 UTC
(In reply to Andrew Pinski from comment #1)
> To fix this you need to have the visibility default attribute on the class
> Archive .

And not just on factory.
Comment 3 Jan Engelhardt 2017-06-08 09:33:34 UTC
The question is more like - can it be made to work even if (In reply to Andrew Pinski from comment #1)
> 
> That is Archive in the shared library and in the main executable are
> considered two different classes.

If that is how UBSAN thinks, then it should say so by uniquely identifying each "distinct" class, e.g.:

main.cpp:3:16: runtime error: member call on address 0x000000dcfc20 which does not point to an object of type 'Archive.1234'
0x000000dcfc20: note: object is of type 'ArchiveImpl' [Archive.5678]


Secondly, I think that considering them two different classes is only valid in C, but not under the ODR rules of C++.


Finally, nonwithstanding all the above, it would be nice to have some -f option to always make UBSAN treat same-name classes as the same thing irrespective of visibility. Reason: to make UBSAN "work" with this example meant having to edit configure.ac+Makefile.am (of the particular real-world program) to not use "-Wl,--version-script=foo.sym -fvisiblity=hidden" at all, and that was a little more work than just slapping another -f into CXXFLAGS.
Comment 4 Jakub Jelinek 2017-06-08 09:47:42 UTC
(In reply to Jan Engelhardt from comment #3)
> The question is more like - can it be made to work even if (In reply to
> Andrew Pinski from comment #1)
> > 
> > That is Archive in the shared library and in the main executable are
> > considered two different classes.
> 
> If that is how UBSAN thinks, then it should say so by uniquely identifying
> each "distinct" class, e.g.:
> 
> main.cpp:3:16: runtime error: member call on address 0x000000dcfc20 which
> does not point to an object of type 'Archive.1234'
> 0x000000dcfc20: note: object is of type 'ArchiveImpl' [Archive.5678]

Where would those magic numbers come from?  We don't have anything like that.

> Secondly, I think that considering them two different classes is only valid
> in C, but not under the ODR rules of C++.

By using visibility other than default, you intentionally depart from the C++ ODR rules, it is an extension and the classes are not the same anymore in different shared libraries.

> Finally, nonwithstanding all the above, it would be nice to have some -f
> option to always make UBSAN treat same-name classes as the same thing
> irrespective of visibility. Reason: to make UBSAN "work" with this example
> meant having to edit configure.ac+Makefile.am (of the particular real-world
> program) to not use "-Wl,--version-script=foo.sym -fvisiblity=hidden" at
> all, and that was a little more work than just slapping another -f into
> CXXFLAGS.

The point of UBSAN is to diagnose UB, and your testcase has UB.  If you want some -f option, that option is -fvisibility=default, which will make the code well defined and ubsan will not complain about it.
Comment 5 Jan Engelhardt 2017-06-08 10:06:55 UTC
>Where would those magic numbers come from?  We don't have anything like that.

Maybe something similar to .build-id?, i.e. randomly-generated IDs (per .so) that merely serve to distinguish two structs.


>>some -f option to always make UBSAN treat same-name classes as the same thing irrespective of visibility.

I think I now found that as -fvisibility-ms-compat.


>By using visibility other than default, you intentionally depart from the C++ ODR rules

I see. Then however the manpage should perhaps be updated to sport some stronger language, since it currently rather encourages the use of visibility and even lists the few conditions that need to be met.

"""It is strongly recommended that you use this in any shared objects you distribute."""
"""-fvisibility does affect C++ vague linkage entities. This means that, for instance, an exception class that is be thrown between DSOs must be explicitly marked with default visibility so that the type_info nodes are unified between the DSOs."""