On 2025-03-28 Pali Rohár wrote:
> On Saturday 22 March 2025 15:35:48 Lasse Collin wrote:
> > I verified in MSYS2's CLANG64 environment that -fsanitize=function
> > and -fsanitize=undefined do complain on Windows too.
> >
> > file1.c:
> >
> > struct foo { int x; };
> > int foo(struct foo *f);
> >
> > int main(void)
> > {
> > struct foo f = { 5 };
> > int (*cb)(struct foo *f) = &foo;
> > return cb(&f);
> > }
> >
> > file2.c:
> >
> > struct wrongname { int x; };
> >
> > int foo(struct wrongname *f)
> > {
> > return f->x;
> > }
> >
> > Testing:
> >
> > $ clang -O2 -fsanitize=undefined file1.c file2.c
> > $ ./a
> > file1.c:8:12: runtime error: call to function foo through pointer
> > to incorrect function type 'int (*)(struct foo *)'
> > (C:\msys64\home\....): note: foo defined here SUMMARY:
> > UndefinedBehaviorSanitizer: undefined-behavior file1.c:8:12
> >
> > If I build with "clang -O2 -fsanitize=cfi -flto" then the program
> > terminates with an illegal instruction instead.
>
> Ah, that is bad :-( Is not there some attribute to mark the function
> to add either structure name alias or turn off the runtime check for
> particular function?
I tried __attribute__((no_sanitize("cfi"))) and a few related
attributes. These work if used in the calling code, for example, on the
main() function in the above example. They don't help if applied to the
foo() declaration or definition. That is, the attributes don't seem
helpful here. Even if an attribute worked, it would poke a small hole
in the CFI protection.
Clang supports -fsanitize-ignorelist=filename.txt. With a very quick
try, it worked for -fsanitize=undefined but not for =cfi. But even if
it worked, this wouldn't really feel like a real solution.
https://clang.llvm.org/docs/SanitizerSpecialCaseList.html
The sanitizer issue should be easy enough to fix properly. Each stat*.c
would, if needed, define _USE_32BIT_TIME_T and _FILE_OFFSET_BITS, and
then use "struct stat". That is, a similar method as my ftw patch used.
-int __cdecl stat64i32(const char *_Filename, struct _stat64i32 *_Stat)
+int __cdecl stat64i32(const char *_Filename, struct stat *_Stat)
{
char *_path = __mingw_fix_stat_path(_Filename);
if (_path == NULL)
return -1;
- int ret = _stat64i32(_path, _Stat);
+ int ret = _stat64i32(_path, (struct _stat64i32 *)_Stat);
Adding the cast doesn't create a new strict aliasing issue; it only
makes the existing strict aliasing issue easier to see. So the above
idea fixes the -fsanitize problem but doesn't make the strict aliasing
situation better or worse. (I'm not sure right now if attributes can
help with that in this specific situation.)
There would also need to be fifth stat*.c file to provide a stat64()
function that uses "struct stat64". Other than the argument type, it
would be ABI compatible with regular stat64(). Assuming that -fsanitize
isn't a worry when directly calling MS CRT functions, only non-UCRT
builds would need the fifth stat*.c file.
#ifdef _UCRT
// stat64 is redirected in the .def file.
int __cdecl stat64(const char *_Filename, struct stat64 *_Stat);
#else
// Wrapper that converts the argument type to keep -fsanitize=function
// and -fsanitize=cfi happy.
int __cdecl stat64(const char *_Filename, struct stat64 *_Stat)
__MINGW_ASM_CALL(__mingw_lfs_stat64);
#endif
> > OK, sorry. I had thought that the misuse of __CRT__NO_INLINE in
> > <sys/stat.h> was a somewhat severe bug, and fixing it as a side
> > effect could have been easy enough here.
>
> As other people pointed out, this needs to be fixed. I proposed one
> solution in other thread.
>
> > In <ftw.h> I used __CRT_BUILDING_FTW. It can be renamed, of course.
> > A separate macro for <ftw.h> is convenient because building ftw*.c
> > depends on the seeing the correct stat struct and function from
> > <sys/stat.h>.
>
> I'm proposing alternative solution to this ftw problem, without
> introduction of any new macro. What do you think about it?
>
> It is attached in ftw.patch file.
I suspect it doesn't work if we care about -fsanitize. The problem
is the callback function provided by the application. It takes a
pointer to struct stat, but with your patch the struct name is not
"stat".
On the other hand, -fsanitize with ftw() (but not nftw()) is already
broken because do_it() always calls the callback function with four
arguments. It's easy, if slightly ugly, to fix by adding another
function pointer to struct ctx_t, and then picking the right one with a
helper function.
If the above aren't a concern, your patch seems fine to me. :-) However,
I'm not sure what else it truly fixes than the ABI symbol names and
avoids new macros in the public headers. If my idea (2) from the other
thread was used, the beginning of ftw.c could have
#define __MINGW_CRTBLD_STAT_VISIBLE
and <ftw.h> could hide the declarations behind _CRTBLD. So
__CRT_BUILDING_FTW wouldn't be needed. The only new macro would be
__MINGW_CRTBLD_STAT_VISIBLE.
> Also it fixes the ABI problem which you have there. In both 32-bit
> msvcrt and 32-bit UCRT builds is "stat" symbol ABI compatible with
> stat32. Even the fact that 32-bit UCRT uses stat function API
> compatible with stat64i32. So it is needed to distinguish between ABI
> symbols and API functions. In your original change you used 32-bit
> UCRT ftw symbol compatible with ftw64i32, which breaks API/ABI
> compatibility with stat symbol and struct stat.
Thanks! I should have copied the #ifdefs from your stat*.c files.
--
Lasse Collin
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
