On Thu, 16 Jan 2025, Lasse Collin wrote:
This series has a few errno handling fixes, error checking fixes,
best-fit mapping avoidance, "long path aware" support, and minor
cleanups. There has been some discussion about best-fit issues
in the "getmainargs changes" thread and on IRC.
I wonder how big problem the ABI breakage in patches 6-7 is. If
a library has DIR* or "struct dirent" in their public API & ABI,
things break if such a pointer is passed between two components
that were built with different dirent code, because each library
or executable contains its own statically-linked copy.
I think that practically, the risk for ABI breakage is very low - it feels
like a highly unusual thing to expose in the public API of a component.
The main thing I could see happening is that something is exposing a
"struct dirent*" out of e.g. a DLL; this would be an issue if this struct
changes layout, but it mostly doesn't - it only grows, and as long as the
recipient doesn't use static copies of such structs and only accesses the
ones returned by readdir, this change should be fine.
If we'd support readdir_r, the ABI would be much more of a concern. But as
we don't, the actual size of struct dirent and the d_name field is mostly
not important anywhere. (One could maybe even consider the struct to be a
struct with an unspecified length of the d_name field? That'd make it
clearer to callers that they'd better not try to allocate it themselves
with a static size.)
Reordering the members in the new DIR might make it fully compatible
with the old closedir(). It might also be doable to make the old
readdir() return NULL with zero errno. So if new opendir() was used,
then old readdir() would falsely show the dir as empty and then
closedir() would close it properly. Is that better or worse than
a crash or other weird behavior? Compatibility the other way
around I didn't think at all yet. None of this is attemped in
this patchset.
I think all of this is overkill. As far as I see it, the primary concern
is ease of reading the implementation code, for verifying correctness.
Changing NAME_MAX may cause bad breakage for similar reasons.
I didn't test rewinddir(), telldir(), or seekdir(). The changes to
those are very simple though so new bugs should be unlikely.
The _WDIR API I only tested that it compiles.
If the patches 1-4 look OK, it's OK to me if they are merged.
There rest should be discussed at least a little. For example,
a mode might be needed that allows best-fit, so reserving
a flag variable for it in the DIR structure could be good.
I'm mostly ok with patches 1-4, the only thing I'd like to clarify is the
discussion about whether POSIX really promises to never touch errno on
success.
With the exception of patch 6, the rest of the patches also look quite
straightforward.
Patch 6, which admittedly is the core of the whole patchset, does add a
fair bit of complexity.
I remember there was a discussion earlier about e.g. the use of
WC_NO_BEST_FIT_CHARS to WideCharToMultiByte wrt support for older
versions of Windows - is this fine for us to use, or do we need to try to
only use it when supported?
As for allowing best-fit mapping; I don't think we should start trying to
have users set flag fields in the DIR structure (it's supposed to be
opaque anyway, as this patchset does). For an API like this, the thing we
provide should be reasonable out of the box without extra configuration.
Other things to consider; some of the changes start relying on _tfullpath
doing malloc if the first parameter is null. Do older CRTs also do this,
or is this a feature that was introduced at some point?
All in all; the patchset looks mostly ok I think. In general I'm not a fan
of added complexity (and this particular API doesn't strike me as the most
pressing part of the worstfit vulnerability), but the fix looks mostly
safe and straightforward as well. What do others think - Liu and Jacek?
I don't have a lot of practical code using these APIs as test case though,
so I hope you've done some practical sanity testing of it.
// Martin
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
