Update OpenBSD glob implementation for Windows #16948

NattyNarwhal · 2024-11-26T18:05:09Z

We're considering making this used as a glob implementation on POSIX as well, but first, we should rebase it from the latest version of OpenBSD.

This also adds a new internal header (charclass.h) for glob.

See conversation in GH-15564.

NattyNarwhal · 2024-11-26T22:06:19Z

MSVC error there is misleading, it's missing reallocarray (and defaults to int(void) I assume). Would need to bring that over, adapt it to use realloc (safely avoiding overflow with args), or convert it ZendMM which has eqivalents anyways.

cmb69 · 2024-11-28T17:54:35Z

MSVC error there is misleading, it's missing reallocarray (and defaults to int(void) I assume).

See #16986.

or convert it ZendMM which has eqivalents anyways.

Might be best, unless that would be a PITA when pulling in further updates.

NattyNarwhal · 2024-11-28T18:51:07Z

For now I imported reallocarray. I don't think glob changes much, so future changes from OpenBSD should be easy to reconcile even if we do convert to ZendMM in the future. Now, to figure out what's broken...

cmb69 · 2024-11-30T12:24:13Z

The usage of SEP is an issue (Windows is fine with forward slashes, too). The following patch solves some problems:

 win32/glob.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/win32/glob.c b/win32/glob.c
index 0923d49efa..36c1c58f1c 100644
--- a/win32/glob.c
+++ b/win32/glob.c
@@ -702,7 +702,7 @@ glob2(Char *pathbuf, Char *pathbuf_last, Char *pathend, Char *pathend_last,
 		/* Find end of next segment, copy tentatively to pathend. */
 		q = pathend;
 		p = pattern;
-		while (*p != EOS && *p != SEP) {
+		while (*p != EOS && !IS_SLASH(*p)) {
 			if (ismeta(*p))
 				anymeta = 1;
 			if (q+1 > pathend_last)
@@ -713,7 +713,7 @@ glob2(Char *pathbuf, Char *pathbuf_last, Char *pathend, Char *pathend_last,
 		if (!anymeta) {		/* No expansion, do next segment. */
 			pathend = q;
 			pattern = p;
-			while (*pattern == SEP) {
+			while (IS_SLASH(*pattern)) {
 				if (pathend+1 > pathend_last)
 					return (1);
 				*pathend++ = *pattern++;

Likely the SEP in line 689 needs to be replace with IS_SLASH() as well.

NattyNarwhal · 2024-12-02T14:50:14Z

Thanks for trying to catch these. I was going to set up a Windows build environment over the weekend to avoid CI driven development, but didn't have any time and my Windows PC (basically used for games nowadays) seems a bit cooked.

cmb69 · 2024-12-02T16:24:42Z

I'm happy to help with this (less platform specific behavior is always nice).

I think this also needs e42aae1, and should be good then.

NattyNarwhal · 2024-12-02T20:21:34Z

I'm also considering porting fnmatch from latest OpenBSD. We currently import an ancient version from FreeBSD, but that involves pulling in even more headers (for i.e. collate, we did so for OpenBSD's charclass.h).

From OpenBSD, dependency of newer versions of glob.

We're considering making this used as a glob implementation on POSIX as well, but first, we should rebase it from the latest version of OpenBSD. This also adds a new internal header (charclass.h) for glob. See conversation in phpGH-15564. Co-authored-by: Christoph M. Becker <[email protected]>

cmb69 · 2024-12-03T16:06:15Z

Just noticed that glob() does not support long and UTF-8 paths on Windows. Sigh.

NattyNarwhal · 2024-12-03T18:40:42Z

Would it just be a matter of making it use the PHP VCWD functions? glob seems abstracted enough internally we can probably change i.e. g_lstat, or provide callbacks in the glob_t at callers/

NattyNarwhal · 2024-12-05T14:41:33Z

Looking at VCWD, I'm not sure if it'd solve it; the stat function already binds to ioutil, lstat doesn't seem to, and while opendir does go through the path resolution code in VCWD, I'm not sure if that would handle the \\?\ type paths (plus there's the MAXPATHLEN stuff). But if you want to try...

diff --git a/win32/glob.c b/win32/glob.c
index f1e75de7344..6435d8f17d2 100644
--- a/win32/glob.c
+++ b/win32/glob.c
@@ -1041,7 +1041,7 @@ g_opendir(Char *str, glob_t *pglob)
 	if (pglob->gl_flags & GLOB_ALTDIRFUNC)
 		return((*pglob->gl_opendir)(buf));
 
-	return(opendir(buf));
+	return(VCWD_OPENDIR(buf));
 }
 
 static int
@@ -1053,7 +1053,7 @@ g_lstat(Char *fn, zend_stat_t *sb, glob_t *pglob)
 		return(-1);
 	if (pglob->gl_flags & GLOB_ALTDIRFUNC)
 		return((*pglob->gl_lstat)(buf, sb));
-	return(php_sys_lstat(buf, sb));
+	return(VCWD_LSTAT(buf, sb));
 }
 
 static int
@@ -1065,7 +1065,7 @@ g_stat(Char *fn, zend_stat_t *sb, glob_t *pglob)
 		return(-1);
 	if (pglob->gl_flags & GLOB_ALTDIRFUNC)
 		return((*pglob->gl_stat)(buf, sb));
-	return(php_sys_stat(buf, sb));
+	return(VCWD_STAT(buf, sb));
 }
 
 static Char *

cmb69 · 2024-12-06T13:16:11Z

I wouldn't worry about UTF-8 and long path support right now (i.e. for the update). There are already a couple of things that are not supported, e.g.

relative paths from the root of the current drive; e.g. \dir is the same as C:\dir if the current drive is C:; IIRC this is a general issue (only supported by NTS, but not ZTS, or maybe the other way round)
UNC paths (at least \\?\C: won't work)
tilde substitution; for ZTS builds it preprends the absolute path of the CWD, so completely broken; for NTS builds it looks like the respective substitution code is disabled (could probably use HOMEDRIVE and HOMEPATH for plain ~, but expanding ~user would not necessarily work)

Anyway, I tried globbing for ü.txt, and that failed with the old version, but worked with the new, probably because of typedef char Char vs. typedef short Char. That might be a good thing or not; I need to check this more closely.

I shall also check other clients of glob(), such as GlobIterator and the glob:// wrapper. So far only checked zif_glob().

cmb69 · 2024-12-06T15:17:14Z

GlobIterator seems to rely on the glob:// wrapper, so doesn't need to be check explicitly. However, the glob:// wrapper has at least bug #17067 (I haven't checked further, yet).

Girgias

Thank you for taking care of this.

I'm not a massive fan of adding a new file for reallocarray() but that's not really a problem.
Left some minor comments

win32/glob.h

win32/glob.c

Girgias · 2024-12-06T18:21:30Z

win32/glob.c

-					(c = qpatnext[1]) != RBRACKET) {
+				    (c = qpatnext[1]) != RBRACKET) {


Indent changed from tab to spaces

Ditto, this how OpenBSD indented it (same for the ones you mentioned below). I can change this to match PHP style.

I prefer to keep this file as close to the original as possible, to make backports from upstream easier.

I don't mind doing the indentation changes if that's how we do it; the file doesn't change too much and in the future we can probably look at backporting individual patches. I just did a blanket import since it looks like we had almost 20 years of changes to deal with.

I prefer to keep this file as close to the original as possible, to make backports from upstream easier.

Well, I suppose if we can backport the indent issues I'm fine with that too

I wouldn't worry that much about diverging from upstream as we might want to do some changes into it eventually. It would be however useful to track the version that it is based on so we can from time to time review changes in the upstream and backport it to our implementation.

Girgias · 2024-12-06T18:21:43Z

win32/glob.c

+		    ((pglob->gl_flags & GLOB_NOMAGIC) &&
+		    !(pglob->gl_flags & GLOB_MAGCHAR)))


Ditto indent changes

Girgias · 2024-12-06T18:21:54Z

win32/glob.c

+	    pathbuf, pathbuf+PATH_MAX-1,
+	    pattern, pattern_last, pglob, limitp));


Girgias · 2024-12-06T18:22:07Z

win32/glob.c

+			    pathend[-1] != SEP) && (S_ISDIR(sb.st_mode) ||
+			    (S_ISLNK(sb.st_mode) &&
+			    (g_stat(pathbuf, &sb, pglob) == 0) &&
+			    S_ISDIR(sb.st_mode)))) {


Indent changes

Girgias · 2024-12-06T18:22:20Z

win32/glob.c

+			    pathend_last, pattern, p, pattern_last,
+			    pglob, limitp));


Girgias · 2024-12-06T18:22:31Z

win32/glob.c

-				pglob->gl_flags & GLOB_ERR)
+			    pglob->gl_flags & GLOB_ERR)


Girgias · 2024-12-06T18:22:42Z

win32/glob.c

-			restpattern, restpattern_last, pglob, limitp);
+		    restpattern, restpattern_last, pglob, limitp);


bukka

Nice first step. Except that reallocarray placement, it looks good to me. It would be good to wait for @cmb69 to do his testing though.

bukka · 2024-12-08T14:36:25Z

main/reallocarray.c

@@ -0,0 +1,59 @@
+/*


I think this should not be in main but in win32. I don't think we will want to use it in this format if glob is made common. In such case we should probably look to introducing something in zend_alloc that would respect memory_limit at some point. I know that the libc glob does not do that but it would be good thing to do eventually.

I imported reallocarray there since it's consistent with other imported utility functions like strlcpy, and we'll need it when we use this glob on not-Windows. ZendMM conversion would render this moot though.

OpenBSD uses tabs for indentation, but four spaces to align i.e. continuations of lines. In PHP, we apparently just use tabs, so do that for code bodies and function declarations.

Matches the style of glob before, OpenBSD must have changed this to match their convention, but this is closer to PHP convention.

NattyNarwhal · 2024-12-09T16:29:25Z

I've cleaned up the indentation stuff and did try to fix some things to match PHP house style. Let me know if I missed something.

If we want to do ZendMM conversion, should that be done in another PR or in this PR? Latter would mean we do even more stuff in this PR, but we can avoid having to import reallocarray just to get rid of it in a ZendMM conversion.

bukka · 2024-12-15T14:16:34Z

As it's just temporary, it should be fine to chuck static php_reallocarray to glob.c and then in the future PR do the Zend MM conversion and get rid of it. You can just add a comment that it's a temp thing.

We only need it on Windows, and we want to convert this file to ZendMM later anyways, which has its own equivalent. This also reverts commit a26e92e.

bukka · 2024-12-22T13:37:22Z

It looks fine to me now. It's all win32 so let's wait for @cmb69 to give it a green light.

NattyNarwhal · 2025-01-09T21:27:23Z

Bumping again, but to recap on what might still need to be done if this is to be merged:

Christoph mentioned long path issues, but that this wasn't blocking this PR.
Is indentation fine here?
Do we want to do fnmatch bumping in this or a new PR? This would probably use OpenBSD instead of FreeBSD as the basis to avoid importing even more headers, but should be compatible.
I think consensus is doing ZendMM conversion in a new PR.

Girgias · 2025-01-09T21:56:26Z

I am happy to progress as is if @cmb69 is OK with it :)

cmb69

I think this is good as is, and there is still plenty of time to PHP 8.5.0alpha1 for additional checking.

cmb69 · 2025-01-09T22:59:57Z

win32/glob.c

+# ifndef PATH_MAX
+#  define PATH_MAX MAXPATHLEN
+# endif


I think we should keep an eye on this; AFAIK PATH_MAX is not defined on Windows, so it will be defined here (what we want).

github-actions bot added the ABI break label Nov 26, 2024

NattyNarwhal force-pushed the glob-update branch from 41594b5 to affa4a7 Compare November 27, 2024 20:16

github-actions bot added the Category: Build System label Nov 27, 2024

NattyNarwhal force-pushed the glob-update branch from affa4a7 to 48fbd32 Compare November 27, 2024 23:08

NattyNarwhal and others added 2 commits December 3, 2024 11:58

Add reallocarray to utility functions

a26e92e

From OpenBSD, dependency of newer versions of glob.

NattyNarwhal force-pushed the glob-update branch from d43cf62 to 3aafb04 Compare December 3, 2024 15:59

NattyNarwhal marked this pull request as ready for review December 3, 2024 16:02

NattyNarwhal requested a review from bukka as a code owner December 3, 2024 16:02

NattyNarwhal requested a review from Girgias December 6, 2024 14:33

Girgias reviewed Dec 6, 2024

View reviewed changes

NattyNarwhal added 2 commits December 6, 2024 14:43

remove tab at end

cd73ebd

Convert to standard C99 type

681d860

bukka reviewed Dec 8, 2024

View reviewed changes

NattyNarwhal added 2 commits December 9, 2024 12:22

Change four spaces to tabs

b161035

OpenBSD uses tabs for indentation, but four spaces to align i.e. continuations of lines. In PHP, we apparently just use tabs, so do that for code bodies and function declarations.

Put function definitions associated with body on one line

64ff225

Matches the style of glob before, OpenBSD must have changed this to match their convention, but this is closer to PHP convention.

Move realloc array into win32/glob.c

ce4c833

We only need it on Windows, and we want to convert this file to ZendMM later anyways, which has its own equivalent. This also reverts commit a26e92e.

cmb69 approved these changes Jan 9, 2025

View reviewed changes

		(c = qpatnext[1]) != RBRACKET) {
		(c = qpatnext[1]) != RBRACKET) {

		((pglob->gl_flags & GLOB_NOMAGIC) &&
		!(pglob->gl_flags & GLOB_MAGCHAR)))

		pathbuf, pathbuf+PATH_MAX-1,
		pattern, pattern_last, pglob, limitp));

		restpattern, restpattern_last, pglob, limitp);
		restpattern, restpattern_last, pglob, limitp);

Update OpenBSD glob implementation for Windows #16948

Are you sure you want to change the base?

Update OpenBSD glob implementation for Windows #16948

Conversation

NattyNarwhal commented Nov 26, 2024

NattyNarwhal commented Nov 26, 2024

cmb69 commented Nov 28, 2024

NattyNarwhal commented Nov 28, 2024

cmb69 commented Nov 30, 2024

NattyNarwhal commented Dec 2, 2024

cmb69 commented Dec 2, 2024

NattyNarwhal commented Dec 2, 2024

cmb69 commented Dec 3, 2024

NattyNarwhal commented Dec 3, 2024

NattyNarwhal commented Dec 5, 2024

cmb69 commented Dec 6, 2024

cmb69 commented Dec 6, 2024

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NattyNarwhal Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bukka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NattyNarwhal commented Dec 9, 2024

bukka commented Dec 15, 2024

bukka commented Dec 22, 2024

NattyNarwhal commented Jan 9, 2025

Girgias commented Jan 9, 2025

cmb69 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NattyNarwhal Dec 6, 2024 •

edited

Loading