On Saturday 29 March 2025 18:56:56 Lasse Collin wrote:
> On 2025-03-29 Pali Rohár wrote:
> > 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:
> > > > > 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?
>
> I think it wouldn't help. -fsanitize=cfi requires specifying also
> -flto. On the other hand, -fsanitize=function or -fsanitize=undefined
> don't require -flto.
>
> > > 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.
>
> I might not fully understand how much this matters.
>
> - Currently one doesn't need to build any stat*.c files for UCRT. The
> .def file redirects statxxx to _statxxx.
>
> - The way I see it, having the fstat/stat/wstat symbols matters mostly
> for Autoconf checks. Normal applications rely on <sys/stat.h>,
> and then the default symbol doesn't matter.
>
> <ftw.h> uses "struct stat", not any underscore version. When apps
> cannot get 32-bit time_t variants of "struct stat" from <sys/stat.h>,
Application can get it when building in msvcrt mode. Application cannot
get it when building in UCRT mode. Both clang and gcc allows to chose
build mode at application build time (not just at mingw-w64 build time).
gcc has -mcrtdll= switch and clang has -lmsvc* switches.
> then there should be no need to have such ftw/nftw functions either.
> The silly exception is that one still needs ftw and nftw symbols so
> that Autoconf can find them, but no real program should call ftw or nftw
> without including <ftw.h>. So in this sense I suppose it doesn't matter
> much what function is used to provide the default symbols; they could
> even be stub functions.
>
> I'm probably missing something important. Sorry.
>
> > Strict aliasing issue on particular type could be probably fixed by
> > the __attribute__((__may_alias__)).
>
> It seems to work even with structs. The attribute *must* be somewhere
> after the "struct" keyword like this:
>
> struct __attribute__((__may_alias__)) foo { int x; };
>
> Maybe this could be the way go to be sure that strict aliasing with the
> struct pointer casts won't cause problems. (It doesn't make a difference
> to the -fsanitize issue.)
This can be a next step.
> > > 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?
>
> My thinking was that with UCRT one can cheat: stat64 is handled in a
> .def file, and thus -fsanitize would not know that it actually takes a
> pointer to "struct _stat64" instead of "struct stat64". A sanitizer
> would only be able to use the information it sees in the header file.
Same applies for example also for msvcr120. Anyway, this is really
fragile and just a hack how to obey the CFI checks.
> With MSVCRT, stat64 comes from stat64.c, and if that is built with
> some -fsanitize, then the type mismatch between <sys/stat.h> and
> stat64.c would be detected. The problem would be stat() when it's
> redirected to stat64() because stat64.c expects "struct stat64 *" while
> stat() uses "struct stat *".
When I'm looking at the ftw() implementation, it has already exactly
same problem. It is taking an fcb function parameter of type:
int (*fcb) (const char *, const STRUCT_STAT *, int)
But it calling it as a different signature:
(*ctx->fcb) (buf, st, flag, ftw);
It contains additional argument.
So I think that the ftw code to work with clang already needs to disable
CFI checks.
Hence I would propose to fix the broken stat functions to work without
CFI and later if somebody comes with a suitable solution we can fix it
also for CFI-enabled compilers. Because it needs to handle aliasing of
stat and stat64 structures, together with those symbols, and also the
fcb callback for ftw() and nftw(), and passing to stat functions, for
both msvcrt and UCRT builds. And this seems to be a huge thing.
> I only understand the high level concept. It's clear that things work
> if the function argument typenames always match. I don't understand
> well in which situations it's possible to cheat. It's still unclear how
> much this even matters *in practice* with mingw-w64 at the moment.
> Sorry, I'm not super helpful with this detail. :-/
>
> --
> Lasse Collin
_______________________________________________
Mingw-w64-public mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
