What was the issue with O_EXEC?

Why _GNU_SOURCE in mysys_priv.h? It's already redundantly defined in my_config.h and my_global.h.

It seems like groonga preferred select, when both poll and select were available, and you've changed it to prefer poll. Why?

What's the PATH_MAX issue that you fix? No PATH_MAX on GNU/Hurd? I can simply define PATH_MAX to be 512 if it isn't defined. Would that work for you? Besides, your realpath() changes in NOSYMLINK_FUNCTION_BODY are broken, realpath() needs only one argument being NULL, not both — how would it know what path to convert? — and even if it'd work, you forgot to free buf at the end

I tried to reply by email to jira@mariadb.org but that failed. Pasting my reply here instead.

> Sergei Golubchik commented on MDEV-12778:
> -----------------------------------------
>
> What was the issue with O_EXEC?

The issue is that O_PATH is not defined on GNU/Hurd and O_EXEC is not either
while on FreeBSD O_PATH is not defined but O_EXEC is.

> Why _GNU_SOURCE in mysys_priv.h? It's already redundantly defined in
> my_config.h and my_global.h.

If _GNU_SOURCE is already defined this part is not needed and can safely be
omitted from the patch.

> It seems like groonga preferred select, when both poll and select were
> available, and you've changed it to prefer poll. Why?

Well, the build on GNU/Hurd defined USE_POLL, not USE_SELECT. I did not take the
time to find out how it could happen, since POLL or SELECT can be arbitrarily
chosen. So I chose to correct the conditions for the USE_POLL case to make
things working.

> What's the PATH_MAX issue that you fix? No PATH_MAX on GNU/Hurd? I can
> simply define PATH_MAX to be 512 if it isn't defined. Would that work for
> you? _Besides, your realpath() changes in NOSYMLINK_FUNCTION_BODY are
> broken, realpath() needs only one argument being NULL, not both — how
> would it know what path to convert? — and even if it'd work, you forgot to
> free buf at the end_

PATH_MAX is never and will never be defined on GNU/Hurd it's bad coding to use
it at all, see http://insanecoding.blogspot.se/2007/11/pathmax-simply-isnt.html

realpath(3) use 2 arguments:

char *realpath(const char *path, char *resolved_path)

and the patch use two too:

-  char buf[PATH_MAX+1];                                                 \

-  if (realpath(pathname, buf) == NULL) return -1;                       \

+  char *buf = realpath(NULL, 0);                                        \

+  if (buf == NULL) return -1;                                           \

The usage of the function CREATE_NOSYMLINK_FUNCTION defined in mysys_priv.h
using NOSYMLINK_FUNCTION_BODY are in mysys/my_open.c and mysys/my_delete.c.

The eventual need of freeing the buffer happen at my_delete.c:

CREATE_NOSYMLINK_FUNCTION(

  unlink_nosymlinks(const char *pathname),

  unlinkat(dfd, filename, 0),

  unlink(pathname)

);

There a free(pathname); would be eventually needed. In the original code
pathname is allocated on the stack, while with realpath(NULL, 0) it will be
allocated on the heap. Depending on how much this call is used, an added free
might be needed. You know how the code works, not me. Perhaps you can show me a
test case so I can check the patch with valgrind (on GNU/Linux, unfortunately it
is not available on GNU/Hurd).

Thanks for your review. Hope things are clearer by now. I'm looking forward to
your further comments on this.

Thanks. Few more questions:

• GNU/Hurd has no O_PATH, no O_EXEC. Does it have O_SEARCH?
• Incidentally, are there man pages for GNU/Hurd online? I didn't find anything, that's why I ask silly questions about open(2) flags.
• POLL/SELECT. I was wrong, sorry, groonga prefers poll, and never defines USE_SELECT, if USE_POLL is defined. Do you happen to have poll(2), but neither poll.h nor sys/poll.h? Can you attach your CMakeCache.txt?

• GNU/Hurd has no O_PATH, no O_EXEC. Does it have O_SEARCH?

Sorry no. the O_* definitions are mainly in /usr/include/i386-gnu/bits/fcntl.h

• Incidentally, are there man pages for GNU/Hurd online? I didn't find anything, that's why I ask silly questions about open(2) flags.

Unfortunately the best documentation is the source code itself: gnumach, hurd and glibc.

• POLL/SELECT. I was wrong, sorry, groonga prefers poll, and never defines USE_SELECT, if USE_POLL is defined. Do you happen to have poll(2), but neither poll.h nor sys/poll.h? Can you attach your CMakeCache.txt?

both <poll.h> and <sys/poll.h> are available. <poll.h> contains: #include <sys/poll.h>
CMakeCache.txt attached.

One more question.

The purpose of my_open_parent_dir_nosymlinks() is to take a path and to open one path component after the other using openat(), while making sure that no path component in a symlink. O_PATH, O_SEARCH, O_EXEC — they all serve this goal. Their semantics is different (O_SEARCH/O_EXEC will fail to open a symlink, O_PATH will open the symlink itself, and the next openat() will fail), but the end result is the same — an error, if there is a symlink in the path.

Can this be done with GNU/Hurd? if not, I'll fall back to realpath()/strcmp() approach.

On Hurd there is: /usr/include/i386-gnu/bits/fcntl.h

1. define O_NOFOLLOW 0x00100000 /* Produce ENOENT if file is a symlink. */
   It is also available in GNU/Linux: /usr/include/x86_64-linux-gnu/bits/fcntl-linux.h
2. define O_NOFOLLOW __O_NOFOLLOW /* Do not follow links. */
   I haven't checked on FreeBSD, but it is probably also available there too.

At least the open(2)and openat(2) manpages describes this flag. Maybe usable?

HTH

ping?

Nothing happened yet. See the release schedule at https://jira.mariadb.org and expect some activity here closer to the release date. Discussions take time, so I was asking questions well before the release, but I don't have questions anymore and the fix will happen, probably, within a week before a release.

gnu_srs, now I remember why O_NOFOLLOW alone is not enough. Opening a directory with O_NOFOLLOW will fail if the directory is not readable. But we don't need readable, we only need executable bit, because ultimately we want to open a file inside the directory. Using O_NOFOLLOW would require all path components up to the actual table file be readable by the mysql user.

So, if GNU/Hurd has no way of opening a directory without requiring it to be readable, then I won't be able to use openat on GNU/Hurd.

cmake.patch:

• changes in package_name.cmake seem to be purely cosmetic, right? The package name is not wrong either way.
• changes in dtrace.cmake — why? Dtrace should be disabled automatically if the "dtrace" executable isn't found.

gnu_srs, as I don't have GNU/Hurd, could you please try this patch — hurd-pathmax.diff — and see whether it fixes PATH_MAX problem for you? It only fixes core server, not mroonga. If mroonga will be an issue, just disable it.

The PATH_MAX part of hurd-pathmax.diff would work. But why do you keep the PATH_MAX code for non-hurd? All POSIX.1-2008 compliant systems do support realpath(whatever, NULL) (sorry for the switched arguments in realpath().

Regarding #define _GNU_SOURCE it is needed, otherwise Lmid_t is not defined when including /usr/include/link.h. It is defined in /usr/include/dlfcn.h included by link.h.

For the fsecond part of the path_max.patch you removed,
-#elif defined(O_EXEC) /* FreeBSD */
+#elif defined(O_EXEC) && defined(_FreeBSD_kernel_) /* FreeBSD */
is needed, since O_EXEC is not defined on GNU/Hurd, i.e.
#define O_PATH O_EXEC
fails. So that part cannot be removed either.

For the USE_SELECT parts, the debian package build cannot easily disable the mroonga plugin, it is a separate package being built for arcihtectures: Architecture: any-alpha any-amd64 any-arm any-arm64 any-i386 any-ia64 any-mips64el any-mips64r6el any-mipsel any-mipsr6el any-nios2 any-powerpcel any-ppc64el any-sh3 any-sh4 any-tilegx
where Hurd belongs to any-i386.

I'll propose an updated patch soon.

New patch: path_max_new.patch attached.

I'm sorry, I don't understand.

Regarding #define _GNU_SOURCE it is needed, otherwise Lmid_t is not defined when including /usr/include/link.h. It is defined in /usr/include/dlfcn.h included by link.h.

_GNU_SOURCE is unconditionally defined in my_config.h, which is included at the beginning of my_global.h which is included first into mysys_priv.h. Why would you need to define _GNU_SOURCE in mysys_priv.h again?

For the fsecond part of the path_max.patch you removed,

-#elif defined(O_EXEC) /* FreeBSD */

+#elif defined(O_EXEC) && defined(_FreeBSD_kernel_) /* FreeBSD */

is needed, since O_EXEC is not defined on GNU/Hurd, i.e.

#define O_PATH O_EXEC

fails. So that part cannot be removed either.

I don't understand that either. The code is

#elif defined(O_EXEC) /* FreeBSD */

#define O_PATH O_EXEC

#endif

So, precisely, when O_EXEC is not defined (e.g. on GNU/Hurd) there will be no #define O_PATH O_EXEC.

But why do you keep the PATH_MAX code for non-hurd?

To avoid unnecessary mallocs.

Sorry about O_EXEC: It is defined on GNU/Hurd but defining O_PATH would make my_open_parent_dir_nosymlinks() used in macro NOSYMLINK_FUNCTION_BODY(AT,NOAT) instead of the definition using realpath().

I'll be back on the _GNU_SOURCE later. Have to recompile without _GNU_SOURCE and issue gcc -E ... to see where things go wrong.

Okay, I see. So GNU/Hurd does have O_EXEC. Why do you try to avoid my_open_parent_dir_nosymlinks()? Because O_EXEC has a different semantics on GNU/Hurd? What semantics is it? May be, because GNU/Hurd does not have openat?

From what I remember that function, my_open_parent_dir_nosymlinks(), does introduce another PATH_MAX issue (sigh). I did not invest the time to avoid another PATH_MAX issue by making it PATH_MAX -free. OK; I was lazy, admitted.

(Will you ever learn not to use PATH_MAX? Perhaps using a fixed size if you want to avoid malloc) See http://insanecoding.blogspot.se/2007/11/pathmax-simply-isnt.html again)

Okay. I've fixed my_open_parent_dir_nosymlinks() not to use PATH_MAX. You should try to use my_open_parent_dir_nosymlinks() if possible, because it's the only race-condition safe way to open symlinked tables.

As far as your poll/select changes are concerned, I don't understand them either
You've changed

#ifdef USE_SELECT

  /* use select */

#else

# ifdef USE_EPOLL

  /* use epoll */

# else

#  ifdef USE_KQUEUE

  /* use kqueue */

#  else

  /* use poll */

#  endif

# endif

#endif

to

#ifdef USE_SELECT

  /* use select */

#else

# ifdef USE_POLL

  /* use poll */

# else

#  ifdef USE_EPOLL

  /* use epoll */

#  else

    /* use kqueue */

#  endif

# endif

#endif

but that couldn't have made any difference, because Groonga always defines only one of the four USE_SELECT, USE_POLL, USE_EPOLL, USE_KQUEUE.

In your case, judging from your CMakeCache.txt, you should have only USE_POLL defined.

gnu_srs, I'm closing this issue.The subject says "due to use of PATH_MAX" and PATH_MAX related changes are in 5.5.57. 5.5.57 is released now and we cannot have this issue "half-fixed" in one version and "rest-fixed" in the other.

So let's close this one as "PATH_MAX" problem. And if you need poll/select changes, please, open a new issue for them (and mention this one, so that you wouldn't need to describe everything again).