On Saturday 29 March 2025 12:19:19 Lasse Collin wrote:
> 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.
Would it help if we compile stat*.c mingw-w64 source files with -fno-lto
option? Or this also does not work?
> 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.
Problem is that you cannot define _USE_32BIT_TIME_T on UCRT builds.
There are header ifdef checks that combination of _USE_32BIT_TIME_T and
_UCRT throw preprocessor #error. But UCRT has _stat32 and also
_stat32i64 functions. So this would not work.
> -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.)
Strict aliasing issue on particular type could be probably fixed by the
__attribute__((__may_alias__)).
> 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
This is for sure wrong. UCRT and msvcrt has same ABI for stat*
functions. So any such ifdef looks very suspicious. Why two libraries
with same ABI requires differences in the header?
> > > 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
