[PATCH] findprog-in: Set errno to indicate why NULL was returned.

[PATCH] findprog-in: Set errno to indicate why NULL was returned.

[Paul Smith]

Set errno to either ENOENT if the program was not found, or another
error if a program was found but was no suitable (i.e., EACCES).

* modules/findprog: Depend on errno.
* lib/findprog-in.c (find_in_given_path): Save errno if it is not ENOENT
and reset errno before returning NULL.
* lib/findprog.h (find_in_given_path): Update the documentation.
---
 lib/findprog-in.c   |  9 +++++++++
 lib/findprog.h      | 20 ++++++++++++--------
 modules/findprog-in |  1 +
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/lib/findprog-in.c b/lib/findprog-in.c
index d601e060d..8ea03f8f7 100644
--- a/lib/findprog-in.c
+++ b/lib/findprog-in.c
@@ -24,6 +24,7 @@
 #include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
+#include <errno.h>
 #include <unistd.h>
 
 #include "filename.h"
@@ -74,6 +75,8 @@ const char *
 find_in_given_path (const char *progname, const char *path,
                     bool optimize_for_exec)
 {
+  int saved_errno = ENOENT;
+
   {
     bool has_slash = false;
     {
@@ -142,11 +145,14 @@ find_in_given_path (const char *progname, const char *path,
                         else
                           return progpathname;
                       }
+                    else if (errno != ENOENT)
+                      saved_errno = errno;
 
                     free (progpathname);
                   }
               }
 
+            errno = saved_errno;
             return NULL;
           }
       }
@@ -221,6 +227,8 @@ find_in_given_path (const char *progname, const char *path,
                     free (path_copy);
                     return progpathname;
                   }
+                else if (errno != ENOENT)
+                  saved_errno = errno;
 
                 free (progpathname);
               }
@@ -234,5 +242,6 @@ find_in_given_path (const char *progname, const char *path,
     free (path_copy);
   }
 
+  errno = saved_errno;
   return NULL;
 }
diff --git a/lib/findprog.h b/lib/findprog.h
index f7b44071f..b1b6caa5f 100644
--- a/lib/findprog.h
+++ b/lib/findprog.h
@@ -41,14 +41,18 @@ extern const char *find_in_path (const char *progname);
    directory.  A null PATH is equivalent to an empty PATH, that is, to the
    singleton list that contains only the current directory.
    Determines the pathname that would be called by execlp/execvp of PROGNAME.
-   - If successful, it returns a pathname containing a slash (either absolute
-     or relative to the current directory).  The returned string can be used
-     with either execl/execv or execlp/execvp.  It is freshly malloc()ed if it
-     is != PROGNAME.
-   - Otherwise, it returns NULL.
-   If OPTIMIZE_FOR_EXEC is true, the function saves some work, under the
-   assumption that the resulting pathname will not be accessed directly,
-   only through execl/execv or execlp/execvp.  */
+   On systems which support executable suffixes these are checked, if PROGNAME
+   does not already contain a suffix.
+   Returns either PROGNAME, or a freshly malloc()ed string, or NULL.  If NULL
+   is returned then errno is set (ENOENT, EACCES, etc.)
+   If PROGNAME contains a slash ('/' on POSIX; '/' or '\' on Windows) then:
+   - If OPTIMIZE_FOR_EXEC is true returns PROGNAME without checking suffixes
+     or existence under the assumption that the resulting pathname will be
+     used with execl/execv/execlp/execvp.
+   - Else if PROGNAME refers an executable program it is returned.
+   - Else NULL is returned and errno is set.
+   Otherwise PATH is searched and the pathname is returned if found, or
+   returns NULL and errno is set.  */
 extern const char *find_in_given_path (const char *progname, const char *path,
                                        bool optimize_for_exec);
 
diff --git a/modules/findprog-in b/modules/findprog-in
index ce7faa50e..d38e6f2be 100644
--- a/modules/findprog-in
+++ b/modules/findprog-in
@@ -9,6 +9,7 @@ m4/eaccess.m4
 
 Depends-on:
 stdbool
+errno
 filename
 xalloc
 xconcat-filename
-- 
2.18.0

Re: [PATCH] findprog-in: Set errno to indicate why NULL was returned.

[Paul Smith]

On Sat, 2019-09-14 at 15:35 -0400, Paul Smith wrote:
> Set errno to either ENOENT if the program was not found, or another
> error if a program was found but was no suitable (i.e., EACCES).

Without this change it's impossible for a program to show the correct
error message when the program is found on the path, but is not
executable.

new module 'access'

[Bruno Haible]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

Paul Smith wrote:
> * lib/findprog-in.c (find_in_given_path): Save errno if it is not ENOENT

While testing whether 'access' really sets errno on native Windows, I found
that:
  - Yes, it does so.
  - But access() with argument X_OK crashes on MSVC. On mingw, it works only
    because mingw links to an older version of the runtime library than MSVC.

So, I'm adding a new module 'access' that makes access(file,X_OK) work
portably.


2019-09-15  Bruno Haible  <bruno@clisp.org>

	access: New module.
	* lib/unistd.in.h (access): New declaration.
	* lib/access.c: New file.
	* m4/access.m4: New file.
	* m4/unistd_h.m4 (gl_UNISTD_H): Test whether access is declared.
	(gl_UNISTD_H_DEFAULTS): Initialize GNULIB_ACCESS, REPLACE_ACCESS.
	* modules/unistd (Makefile.am): Substitute GNULIB_ACCESS,
	REPLACE_ACCESS.
	* modules/access: New file.
	* tests/test-unistd-c++.cc (access): Check signature.
	* doc/posix-functions/access.texi: Mention the new module.

	access: Add tests.
	* tests/test-access.c: New file.
	* modules/access-tests: New file.

	findprog, findprog-lgpl, findprog-in: Fix crash on MSVC.
	* modules/findprog (Depends-on): Add access.
	* modules/findprog-lgpl (Depends-on): Likewise.
	* modules/findprog-in (Depends-on): Likewise.

[-- Attachment #2: 0001-access-New-module.patch --]
[-- Type: text/x-patch, Size: 9563 bytes --]

From 265886a27c8bc637b1e1f3e85e68d594bbe5fa2c Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sun, 15 Sep 2019 18:56:46 +0200
Subject: [PATCH 1/3] access: New module.

* lib/unistd.in.h (access): New declaration.
* lib/access.c: New file.
* m4/access.m4: New file.
* m4/unistd_h.m4 (gl_UNISTD_H): Test whether access is declared.
(gl_UNISTD_H_DEFAULTS): Initialize GNULIB_ACCESS, REPLACE_ACCESS.
* modules/unistd (Makefile.am): Substitute GNULIB_ACCESS,
REPLACE_ACCESS.
* modules/access: New file.
* tests/test-unistd-c++.cc (access): Check signature.
* doc/posix-functions/access.texi: Mention the new module.
---
 ChangeLog                       | 14 ++++++++++++++
 doc/posix-functions/access.texi |  5 ++++-
 lib/access.c                    | 31 +++++++++++++++++++++++++++++++
 lib/unistd.in.h                 | 22 ++++++++++++++++++++--
 m4/access.m4                    | 16 ++++++++++++++++
 m4/unistd_h.m4                  | 10 ++++++----
 modules/access                  | 28 ++++++++++++++++++++++++++++
 modules/unistd                  |  4 +++-
 tests/test-unistd-c++.cc        |  4 ++++
 9 files changed, 126 insertions(+), 8 deletions(-)
 create mode 100644 lib/access.c
 create mode 100644 m4/access.m4
 create mode 100644 modules/access

diff --git a/ChangeLog b/ChangeLog
index 84d9508..b85c3dd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
 2019-09-15  Bruno Haible  <bruno@clisp.org>
 
+	access: New module.
+	* lib/unistd.in.h (access): New declaration.
+	* lib/access.c: New file.
+	* m4/access.m4: New file.
+	* m4/unistd_h.m4 (gl_UNISTD_H): Test whether access is declared.
+	(gl_UNISTD_H_DEFAULTS): Initialize GNULIB_ACCESS, REPLACE_ACCESS.
+	* modules/unistd (Makefile.am): Substitute GNULIB_ACCESS,
+	REPLACE_ACCESS.
+	* modules/access: New file.
+	* tests/test-unistd-c++.cc (access): Check signature.
+	* doc/posix-functions/access.texi: Mention the new module.
+
+2019-09-15  Bruno Haible  <bruno@clisp.org>
+
 	fcntl-h: Fix compilation error of creat.c on MSVC.
 	* lib/fcntl.in.h: Include <io.h> also when __need_system_fcntl_h is
 	defined.
diff --git a/doc/posix-functions/access.texi b/doc/posix-functions/access.texi
index fe66ae0..d9c5006 100644
--- a/doc/posix-functions/access.texi
+++ b/doc/posix-functions/access.texi
@@ -4,10 +4,13 @@
 
 POSIX specification:@* @url{http://www.opengroup.org/onlinepubs/9699919799/functions/access.html}
 
-Gnulib module: ---
+Gnulib module: access
 
 Portability problems fixed by Gnulib:
 @itemize
+@item
+This function does not support the @code{X_OK} mode on some platforms:
+MSVC 14.
 @end itemize
 
 Portability problems not fixed by Gnulib:
diff --git a/lib/access.c b/lib/access.c
new file mode 100644
index 0000000..210f7f4
--- /dev/null
+++ b/lib/access.c
@@ -0,0 +1,31 @@
+/* Test the access rights of a file.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+#include <config.h>
+
+/* Specification.  */
+#include <unistd.h>
+
+#include <fcntl.h>
+#include <io.h>
+
+int
+access (const char *file, int mode)
+{
+  if ((mode & X_OK) != 0)
+    mode = (mode & ~X_OK) | R_OK;
+  return _access (file, mode);
+}
diff --git a/lib/unistd.in.h b/lib/unistd.in.h
index 032cc93..eaf734d 100644
--- a/lib/unistd.in.h
+++ b/lib/unistd.in.h
@@ -253,10 +253,28 @@ _GL_INLINE_HEADER_BEGIN
 /* Declare overridden functions.  */
 
 
-#if defined GNULIB_POSIXCHECK
+#if @GNULIB_ACCESS@
+# if @REPLACE_ACCESS@
+#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
+#   undef access
+#   define access rpl_access
+#  endif
+_GL_FUNCDECL_RPL (access, int, (const char *file, int mode)
+                               _GL_ARG_NONNULL ((1)));
+_GL_CXXALIAS_RPL (access, int, (const char *file, int mode));
+# else
+_GL_CXXALIAS_SYS (access, int, (const char *file, int mode));
+# endif
+_GL_CXXALIASWARN (access);
+#elif defined GNULIB_POSIXCHECK
+# undef access
+# if HAVE_RAW_DECL_ACCESS
 /* The access() function is a security risk.  */
-_GL_WARN_ON_USE (access, "the access function is a security risk - "
+_GL_WARN_ON_USE (access, "access does not always support X_OK - "
+                 "use gnulib module access for portability; "
+                 "also, this function is a security risk - "
                  "use the gnulib module faccessat instead");
+# endif
 #endif
 
 
diff --git a/m4/access.m4 b/m4/access.m4
new file mode 100644
index 0000000..a718f81
--- /dev/null
+++ b/m4/access.m4
@@ -0,0 +1,16 @@
+# access.m4 serial 1
+dnl Copyright (C) 2019 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+AC_DEFUN([gl_FUNC_ACCESS],
+[
+  AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
+  AC_REQUIRE([AC_CANONICAL_HOST])
+  dnl On native Windows, access (= _access) does not support the X_OK mode.
+  dnl It works by chance on some versions of mingw.
+  case "$host_os" in
+    mingw*) REPLACE_ACCESS=1 ;;
+  esac
+])
diff --git a/m4/unistd_h.m4 b/m4/unistd_h.m4
index a3b3905..18b7140 100644
--- a/m4/unistd_h.m4
+++ b/m4/unistd_h.m4
@@ -1,4 +1,4 @@
-# unistd_h.m4 serial 75
+# unistd_h.m4 serial 76
 dnl Copyright (C) 2006-2019 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -41,9 +41,9 @@ AC_DEFUN([gl_UNISTD_H],
 #  include <io.h>
 # endif
 #endif
-    ]], [chdir chown dup dup2 dup3 environ euidaccess faccessat fchdir fchownat
-    fdatasync fsync ftruncate getcwd getdomainname getdtablesize getgroups
-    gethostname getlogin getlogin_r getpagesize getpass
+    ]], [access chdir chown dup dup2 dup3 environ euidaccess faccessat fchdir
+    fchownat fdatasync fsync ftruncate getcwd getdomainname getdtablesize
+    getgroups gethostname getlogin getlogin_r getpagesize getpass
     getusershell setusershell endusershell
     group_member isatty lchown link linkat lseek pipe pipe2 pread pwrite
     readlink readlinkat rmdir sethostname sleep symlink symlinkat
@@ -61,6 +61,7 @@ AC_DEFUN([gl_UNISTD_MODULE_INDICATOR],
 
 AC_DEFUN([gl_UNISTD_H_DEFAULTS],
 [
+  GNULIB_ACCESS=0;               AC_SUBST([GNULIB_ACCESS])
   GNULIB_CHDIR=0;                AC_SUBST([GNULIB_CHDIR])
   GNULIB_CHOWN=0;                AC_SUBST([GNULIB_CHOWN])
   GNULIB_CLOSE=0;                AC_SUBST([GNULIB_CLOSE])
@@ -159,6 +160,7 @@ AC_DEFUN([gl_UNISTD_H_DEFAULTS],
   HAVE_DECL_TTYNAME_R=1;  AC_SUBST([HAVE_DECL_TTYNAME_R])
   HAVE_OS_H=0;            AC_SUBST([HAVE_OS_H])
   HAVE_SYS_PARAM_H=0;     AC_SUBST([HAVE_SYS_PARAM_H])
+  REPLACE_ACCESS=0;       AC_SUBST([REPLACE_ACCESS])
   REPLACE_CHOWN=0;        AC_SUBST([REPLACE_CHOWN])
   REPLACE_CLOSE=0;        AC_SUBST([REPLACE_CLOSE])
   REPLACE_DUP=0;          AC_SUBST([REPLACE_DUP])
diff --git a/modules/access b/modules/access
new file mode 100644
index 0000000..7434e02
--- /dev/null
+++ b/modules/access
@@ -0,0 +1,28 @@
+Description:
+access() function: test the access rights of a file.
+
+Files:
+lib/access.c
+m4/access.m4
+
+Depends-on:
+unistd
+fcntl
+
+configure.ac:
+gl_FUNC_ACCESS
+if test $REPLACE_ACCESS = 1; then
+  AC_LIBOBJ([access])
+fi
+gl_UNISTD_MODULE_INDICATOR([access])
+
+Makefile.am:
+
+Include:
+<unistd.h>
+
+License:
+LGPLv2+
+
+Maintainer:
+all
diff --git a/modules/unistd b/modules/unistd
index e29c1ad..b68029d 100644
--- a/modules/unistd
+++ b/modules/unistd
@@ -36,6 +36,7 @@ unistd.h: unistd.in.h $(top_builddir)/config.status $(CXXDEFS_H) $(ARG_NONNULL_H
 	      -e 's|@''PRAGMA_COLUMNS''@|@PRAGMA_COLUMNS@|g' \
 	      -e 's|@''NEXT_UNISTD_H''@|$(NEXT_UNISTD_H)|g' \
 	      -e 's|@''WINDOWS_64_BIT_OFF_T''@|$(WINDOWS_64_BIT_OFF_T)|g' \
+	      -e 's/@''GNULIB_ACCESS''@/$(GNULIB_ACCESS)/g' \
 	      -e 's/@''GNULIB_CHDIR''@/$(GNULIB_CHDIR)/g' \
 	      -e 's/@''GNULIB_CHOWN''@/$(GNULIB_CHOWN)/g' \
 	      -e 's/@''GNULIB_CLOSE''@/$(GNULIB_CLOSE)/g' \
@@ -135,7 +136,8 @@ unistd.h: unistd.in.h $(top_builddir)/config.status $(CXXDEFS_H) $(ARG_NONNULL_H
 	      -e 's|@''HAVE_OS_H''@|$(HAVE_OS_H)|g' \
 	      -e 's|@''HAVE_SYS_PARAM_H''@|$(HAVE_SYS_PARAM_H)|g' \
 	  | \
-	  sed -e 's|@''REPLACE_CHOWN''@|$(REPLACE_CHOWN)|g' \
+	  sed -e 's|@''REPLACE_ACCESS''@|$(REPLACE_ACCESS)|g' \
+	      -e 's|@''REPLACE_CHOWN''@|$(REPLACE_CHOWN)|g' \
 	      -e 's|@''REPLACE_CLOSE''@|$(REPLACE_CLOSE)|g' \
 	      -e 's|@''REPLACE_DUP''@|$(REPLACE_DUP)|g' \
 	      -e 's|@''REPLACE_DUP2''@|$(REPLACE_DUP2)|g' \
diff --git a/tests/test-unistd-c++.cc b/tests/test-unistd-c++.cc
index e6a6952..8ba7a04 100644
--- a/tests/test-unistd-c++.cc
+++ b/tests/test-unistd-c++.cc
@@ -24,6 +24,10 @@
 #include "signature.h"
 
 
+#if GNULIB_TEST_ACCESS
+SIGNATURE_CHECK (GNULIB_NAMESPACE::access, int, (const char *, int));
+#endif
+
 #if GNULIB_TEST_CHDIR
 SIGNATURE_CHECK (GNULIB_NAMESPACE::chdir, int, (const char *));
 #endif
-- 
2.7.4


[-- Attachment #3: 0002-access-Add-tests.patch --]
[-- Type: text/x-patch, Size: 3477 bytes --]

From 213cd443c2fdae0471e30546755e3cd15fcf7d9d Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sun, 15 Sep 2019 18:58:42 +0200
Subject: [PATCH 2/3] access: Add tests.

* tests/test-access.c: New file.
* modules/access-tests: New file.
---
 ChangeLog            |  4 +++
 modules/access-tests | 13 +++++++++
 tests/test-access.c  | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+)
 create mode 100644 modules/access-tests
 create mode 100644 tests/test-access.c

diff --git a/ChangeLog b/ChangeLog
index b85c3dd..394dcd5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
 2019-09-15  Bruno Haible  <bruno@clisp.org>
 
+	access: Add tests.
+	* tests/test-access.c: New file.
+	* modules/access-tests: New file.
+
 	access: New module.
 	* lib/unistd.in.h (access): New declaration.
 	* lib/access.c: New file.
diff --git a/modules/access-tests b/modules/access-tests
new file mode 100644
index 0000000..082aeb5
--- /dev/null
+++ b/modules/access-tests
@@ -0,0 +1,13 @@
+Files:
+tests/test-access.c
+tests/signature.h
+tests/macros.h
+
+Depends-on:
+creat
+
+configure.ac:
+
+Makefile.am:
+TESTS += test-access
+check_PROGRAMS += test-access
diff --git a/tests/test-access.c b/tests/test-access.c
new file mode 100644
index 0000000..7af7f9a
--- /dev/null
+++ b/tests/test-access.c
@@ -0,0 +1,80 @@
+/* Tests of access.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
+
+#include <config.h>
+
+#include <unistd.h>
+
+#include "signature.h"
+SIGNATURE_CHECK (access, int, (const char *, int));
+
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+#include "macros.h"
+
+#define BASE "test-access.t"
+
+int
+main ()
+{
+  /* Remove anything from prior partial run.  */
+  unlink (BASE "f");
+  unlink (BASE "f1");
+  unlink (BASE "f2");
+
+  {
+    errno = 0;
+    ASSERT (access (BASE "f", R_OK) == -1);
+    ASSERT (errno == ENOENT);
+
+    errno = 0;
+    ASSERT (access (BASE "f", W_OK) == -1);
+    ASSERT (errno == ENOENT);
+
+    errno = 0;
+    ASSERT (access (BASE "f", X_OK) == -1);
+    ASSERT (errno == ENOENT);
+  }
+  {
+    ASSERT (close (creat (BASE "f1", 0700)) == 0);
+
+    ASSERT (access (BASE "f1", R_OK) == 0);
+    ASSERT (access (BASE "f1", W_OK) == 0);
+    ASSERT (access (BASE "f1", X_OK) == 0);
+  }
+  {
+    ASSERT (close (creat (BASE "f2", 0600)) == 0);
+    ASSERT (chmod (BASE "f2", 0400) == 0);
+
+    ASSERT (access (BASE "f2", R_OK) == 0);
+
+    errno = 0;
+    ASSERT (access (BASE "f2", W_OK) == -1);
+    ASSERT (errno == EACCES);
+
+#if defined _WIN32 && !defined __CYGWIN__
+    /* X_OK works like R_OK.  */
+    ASSERT (access (BASE "f2", X_OK) == 0);
+#else
+    errno = 0;
+    ASSERT (access (BASE "f2", X_OK) == -1);
+    ASSERT (errno == EACCES);
+#endif
+  }
+
+  return 0;
+}
-- 
2.7.4


[-- Attachment #4: 0003-findprog-findprog-lgpl-findprog-in-Fix-crash-on-MSVC.patch --]
[-- Type: text/x-patch, Size: 1740 bytes --]

From ccfbc26dce79adcb32bf421ff813247699d70ea5 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sun, 15 Sep 2019 19:00:51 +0200
Subject: [PATCH 3/3] findprog, findprog-lgpl, findprog-in: Fix crash on MSVC.

* modules/findprog (Depends-on): Add access.
* modules/findprog-lgpl (Depends-on): Likewise.
* modules/findprog-in (Depends-on): Likewise.
---
 ChangeLog             | 7 +++++++
 modules/findprog      | 1 +
 modules/findprog-in   | 1 +
 modules/findprog-lgpl | 1 +
 4 files changed, 10 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 394dcd5..7d2eb3a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2019-09-15  Bruno Haible  <bruno@clisp.org>
 
+	findprog, findprog-lgpl, findprog-in: Fix crash on MSVC.
+	* modules/findprog (Depends-on): Add access.
+	* modules/findprog-lgpl (Depends-on): Likewise.
+	* modules/findprog-in (Depends-on): Likewise.
+
+2019-09-15  Bruno Haible  <bruno@clisp.org>
+
 	access: Add tests.
 	* tests/test-access.c: New file.
 	* modules/access-tests: New file.
diff --git a/modules/findprog b/modules/findprog
index c849d66..d48158b 100644
--- a/modules/findprog
+++ b/modules/findprog
@@ -11,6 +11,7 @@ Depends-on:
 stdbool
 xalloc
 xconcat-filename
+access
 unistd
 
 configure.ac:
diff --git a/modules/findprog-in b/modules/findprog-in
index ce7faa5..971366b 100644
--- a/modules/findprog-in
+++ b/modules/findprog-in
@@ -12,6 +12,7 @@ stdbool
 filename
 xalloc
 xconcat-filename
+access
 unistd
 
 configure.ac:
diff --git a/modules/findprog-lgpl b/modules/findprog-lgpl
index 63c78d0..477eccb 100644
--- a/modules/findprog-lgpl
+++ b/modules/findprog-lgpl
@@ -12,6 +12,7 @@ Depends-on:
 stdbool
 strdup
 concat-filename
+access
 unistd
 
 configure.ac:
-- 
2.7.4

Re: [PATCH] findprog-in: Set errno to indicate why NULL was returned.

[Bruno Haible]

Hi Paul,

> Without this change it's impossible for a program to show the correct
> error message when the program is found on the path, but is not
> executable.

Good point!

I've committed a slightly different patch, based on yours. While your
patch was fully correct, I have different preferences:

  - You don't need a dependency to the 'errno' module, because
    (see doc/posix-headers/errno.texi) it is only needed for more
    "advanced" errno values. For the basic ones like ENOENT, EINVAL,
    EACCES, etc. no portability problems are known.

  - Since the errno value is a saved one in some cases, but not in
    all cases, I prefer a variable name 'failure_errno' to 'saved_errno'.

  - In the specification comment in findprog.h: I avoid explaining the
    algorithm that a certain function employs. Only: what are the inputs?
    what are the outputs? and only the minimal necessary information about
    the inner workings of the function.
    It must be possible to describe a function in an abstract way. If I
    was led to document the algorithm that a function uses, this would
    be a sign that the function is not well designed.


2019-09-15  Paul Smith  <psmith@gnu.org>
            Bruno Haible  <bruno@clisp.org>

	findprog-in: Set errno when the search fails.
	* lib/findprog-in.c: Include <errno.h>.
	(find_in_given_path): Set errno before returning NULL.
	* lib/findprog.h (find_in_given_path): Update comment accordingly.
	Define the term "slash".

diff --git a/lib/findprog-in.c b/lib/findprog-in.c
index d601e06..5e90680 100644
--- a/lib/findprog-in.c
+++ b/lib/findprog-in.c
@@ -21,6 +21,7 @@
 /* Specification.  */
 #include "findprog.h"
 
+#include <errno.h>
 #include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
@@ -98,6 +99,7 @@ find_in_given_path (const char *progname, const char *path,
           {
             /* Try the various suffixes and see whether one of the files
                with such a suffix is actually executable.  */
+            int failure_errno;
             size_t i;
             #if defined _WIN32 && !defined __CYGWIN__ /* Native Windows */
             const char *progbasename;
@@ -113,6 +115,7 @@ find_in_given_path (const char *progname, const char *path,
             #endif
 
             /* Try all platform-dependent suffixes.  */
+            failure_errno = ENOENT;
             for (i = 0; i < sizeof (suffixes) / sizeof (suffixes[0]); i++)
               {
                 const char *suffix = suffixes[i];
@@ -143,10 +146,14 @@ find_in_given_path (const char *progname, const char *path,
                           return progpathname;
                       }
 
+                    if (errno != ENOENT)
+                      failure_errno = errno;
+
                     free (progpathname);
                   }
               }
 
+            errno = failure_errno;
             return NULL;
           }
       }
@@ -158,11 +165,13 @@ find_in_given_path (const char *progname, const char *path,
     path = "";
 
   {
+    int failure_errno;
     /* Make a copy, to prepare for destructive modifications.  */
     char *path_copy = xstrdup (path);
     char *path_rest;
     char *cp;
 
+    failure_errno = ENOENT;
     for (path_rest = path_copy; ; path_rest = cp + 1)
       {
         const char *dir;
@@ -222,6 +231,9 @@ find_in_given_path (const char *progname, const char *path,
                     return progpathname;
                   }
 
+                if (errno != ENOENT)
+                  failure_errno = errno;
+
                 free (progpathname);
               }
           }
@@ -232,7 +244,8 @@ find_in_given_path (const char *progname, const char *path,
 
     /* Not found in PATH.  */
     free (path_copy);
-  }
 
-  return NULL;
+    errno = failure_errno;
+    return NULL;
+  }
 }
diff --git a/lib/findprog.h b/lib/findprog.h
index f7b4407..804f02a 100644
--- a/lib/findprog.h
+++ b/lib/findprog.h
@@ -36,19 +36,29 @@ extern "C" {
 extern const char *find_in_path (const char *progname);
 
 /* Looks up a program in the given PATH-like string.
+
    The PATH argument consists of a list of directories, separated by ':' or
    (on native Windows) by ';'.  An empty PATH element designates the current
    directory.  A null PATH is equivalent to an empty PATH, that is, to the
    singleton list that contains only the current directory.
+
    Determines the pathname that would be called by execlp/execvp of PROGNAME.
    - If successful, it returns a pathname containing a slash (either absolute
      or relative to the current directory).  The returned string can be used
      with either execl/execv or execlp/execvp.  It is freshly malloc()ed if it
      is != PROGNAME.
-   - Otherwise, it returns NULL.
+   - Otherwise, it sets errno and returns NULL.
+     Specific errno values include:
+       - ENOENT: means that the program's file was not found.
+       - EACCESS: means that the program's file was found but lacks the
+         execute permissions.
    If OPTIMIZE_FOR_EXEC is true, the function saves some work, under the
    assumption that the resulting pathname will not be accessed directly,
-   only through execl/execv or execlp/execvp.  */
+   only through execl/execv or execlp/execvp.
+
+   Here, a "slash" means:
+     - On POSIX systems excluding Cygwin: a '/',
+     - On Windows, OS/2, DOS platforms: a '/' or '\'. */
 extern const char *find_in_given_path (const char *progname, const char *path,
                                        bool optimize_for_exec);
 

Re: [PATCH] findprog-in: Set errno to indicate why NULL was returned.

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: Paul Smith <psmith@gnu.org>
> Date: Sun, 15 Sep 2019 19:58:56 +0200
> 
> +   - Otherwise, it sets errno and returns NULL.
> +     Specific errno values include:
> +       - ENOENT: means that the program's file was not found.
> +       - EACCESS: means that the program's file was found but lacks the
> +         execute permissions.

The value of EACCES (not EACCESS!) on MS-Windows frequently means the
file cannot be accessed for a reason that has nothing to do with
execute permissions.  It could be, for example, that some intermediate
directory in the full file name doesn't exist or is otherwise
inaccessible.

Re: new module 'access'

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: Paul Smith <psmith@gnu.org>
> Date: Sun, 15 Sep 2019 19:11:25 +0200
> 
> +@item
> +This function does not support the @code{X_OK} mode on some platforms:
> +MSVC 14.

This says MSVC, but the code will do the same on MinGW, right?

> +int
> +access (const char *file, int mode)
> +{
> +  if ((mode & X_OK) != 0)
> +    mode = (mode & ~X_OK) | R_OK;
> +  return _access (file, mode);

This implementation is IMO less useful than it could be: it could also
look at the file-name extension and see if it's one of the known
executable extensions.

Thanks.

Re: new module 'access'

[Bruno Haible]

Hi Eli,

> > +@item
> > +This function does not support the @code{X_OK} mode on some platforms:
> > +MSVC 14.
> 
> This says MSVC, but the code will do the same on MinGW, right?

Yes, I enabled this code also on mingw. With the mingw version I tested,
it is a no-op, because that mingw version links against an older MSVCRT
runtime. If/when mingw starts to link against newer Microsoft ucrt runtimes,
it will be affected by the same problem.

> > +int
> > +access (const char *file, int mode)
> > +{
> > +  if ((mode & X_OK) != 0)
> > +    mode = (mode & ~X_OK) | R_OK;
> > +  return _access (file, mode);
> 
> This implementation is IMO less useful than it could be: it could also
> look at the file-name extension and see if it's one of the known
> executable extensions.

Should it look whether the file name contains a '.'? (Recall that
files without a '.' are not executable by execlp/execvp.) Yes, it could
be done. But I don't see a compelling argument to do it, so far.

Should it look whether the file extension is one of the known ones?
Definitely not. When you rename a file prog.exe to prog.foo and invoke
it through execlp/execvp, it works. And '.foo' is surely not one of the
"known" file extensions.

Bruno

Re: new module 'access'

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: bug-gnulib@gnu.org, psmith@gnu.org
> Date: Tue, 17 Sep 2019 00:45:03 +0200
> 
> > > +@item
> > > +This function does not support the @code{X_OK} mode on some platforms:
> > > +MSVC 14.
> > 
> > This says MSVC, but the code will do the same on MinGW, right?
> 
> Yes, I enabled this code also on mingw. With the mingw version I tested,
> it is a no-op, because that mingw version links against an older MSVCRT
> runtime. If/when mingw starts to link against newer Microsoft ucrt runtimes,
> it will be affected by the same problem.

I'm saying that the documentation should mention MinGW as well,
because currently it gives an impression that only MSVC builds are
affected in any way.

> > > +int
> > > +access (const char *file, int mode)
> > > +{
> > > +  if ((mode & X_OK) != 0)
> > > +    mode = (mode & ~X_OK) | R_OK;
> > > +  return _access (file, mode);
> > 
> > This implementation is IMO less useful than it could be: it could also
> > look at the file-name extension and see if it's one of the known
> > executable extensions.
> 
> Should it look whether the file name contains a '.'? (Recall that
> files without a '.' are not executable by execlp/execvp.) Yes, it could
> be done. But I don't see a compelling argument to do it, so far.
> 
> Should it look whether the file extension is one of the known ones?
> Definitely not. When you rename a file prog.exe to prog.foo and invoke
> it through execlp/execvp, it works. And '.foo' is surely not one of the
> "known" file extensions.

You describe a very unusual situation, because prog.foo will not be
found by the Windows shell and by many other programs that use the
shell via the likes of 'system' and 'popen'.  I think it's better to
be 90% correct than do nothing about this issue because we cannot
easily be 100% correct.  Callers don't usually expect an X_OK test to
degenerate into F_OK, IMO.

IOW, I think this implementation doesn't live up to Gnulib's promise
to be a portability layer, because it loses too much of the baby with
the bath water.

Re: [PATCH] findprog-in: Set errno to indicate why NULL was returned.

[Bruno Haible]

Eli Zaretskii wrote:
> The value of EACCES (not EACCESS!) on MS-Windows frequently means the
> file cannot be accessed for a reason that has nothing to do with
> execute permissions.  It could be, for example, that some intermediate
> directory in the full file name doesn't exist or is otherwise
> inaccessible.

Thanks for the scrutiny, Eli. Fixed as follows:


2019-09-15  Bruno Haible  <bruno@clisp.org>

	findprog-in: Fix comment.
	Reported by Eli Zaretskii <eliz@gnu.org>.
	* lib/findprog.h (find_in_given_path): Extend description of EACCES
	condition.
	* lib/stat.c (rpl_stat): Fix typo in comment.
	* lib/utime.c (_gl_utimens_windows): Likewise.

diff --git a/lib/findprog.h b/lib/findprog.h
index 804f02a..263c53c 100644
--- a/lib/findprog.h
+++ b/lib/findprog.h
@@ -50,8 +50,9 @@ extern const char *find_in_path (const char *progname);
    - Otherwise, it sets errno and returns NULL.
      Specific errno values include:
        - ENOENT: means that the program's file was not found.
-       - EACCESS: means that the program's file was found but lacks the
-         execute permissions.
+       - EACCES: means that the program's file cannot be accessed (due to some
+         issue with one of the ancestor directories) or lacks the execute
+         permissions.
    If OPTIMIZE_FOR_EXEC is true, the function saves some work, under the
    assumption that the resulting pathname will not be accessed directly,
    only through execl/execv or execlp/execvp.
diff --git a/lib/stat.c b/lib/stat.c
index 9228500..e3248a8 100644
--- a/lib/stat.c
+++ b/lib/stat.c
@@ -375,7 +375,7 @@ rpl_stat (char const *name, struct stat *buf)
 
       case ERROR_ACCESS_DENIED:  /* rname is such as 'C:\System Volume Information\foo'.  */
       case ERROR_SHARING_VIOLATION: /* rname is such as 'C:\pagefile.sys' (second approach only).  */
-                                    /* XXX map to EACCESS or EPERM? */
+                                    /* XXX map to EACCES or EPERM? */
         errno = EACCES;
         break;
 
@@ -398,7 +398,7 @@ rpl_stat (char const *name, struct stat *buf)
         errno = ENAMETOOLONG;
         break;
 
-      case ERROR_DELETE_PENDING: /* XXX map to EACCESS or EPERM? */
+      case ERROR_DELETE_PENDING: /* XXX map to EACCES or EPERM? */
         errno = EPERM;
         break;
 
diff --git a/lib/utime.c b/lib/utime.c
index a0466f7..a5a362b 100644
--- a/lib/utime.c
+++ b/lib/utime.c
@@ -224,7 +224,7 @@ _gl_utimens_windows (const char *name, struct timespec ts[2])
         errno = ENAMETOOLONG;
         break;
 
-      case ERROR_DELETE_PENDING: /* XXX map to EACCESS or EPERM? */
+      case ERROR_DELETE_PENDING: /* XXX map to EACCES or EPERM? */
         errno = EPERM;
         break;
 

Re: new module 'access'

[Bruno Haible]

Eli Zaretskii wrote:
> > > > +@item
> > > > +This function does not support the @code{X_OK} mode on some platforms:
> > > > +MSVC 14.
> > > 
> > > This says MSVC, but the code will do the same on MinGW, right?
> > 
> > Yes, I enabled this code also on mingw. With the mingw version I tested,
> > it is a no-op, because that mingw version links against an older MSVCRT
> > runtime. If/when mingw starts to link against newer Microsoft ucrt runtimes,
> > it will be affected by the same problem.
> 
> I'm saying that the documentation should mention MinGW as well,
> because currently it gives an impression that only MSVC builds are
> affected in any way.

So far, only MSVC builds are affected. The gnulib documentation lists
situations/platforms that we _know_ are buggy. There is always the
implicit statement "If a situation/platform is not mentioned explicitly
it may still be buggy". For example, often FreeBSD 6.0 is mentioned to
have a certain bug, although the bug may also exist in FreeBSD 11 and 12
- simply because I don't have the time to verify each bug on each new
version of FreeBSD.

> > Should it look whether the file extension is one of the known ones?
> > Definitely not. When you rename a file prog.exe to prog.foo and invoke
> > it through execlp/execvp, it works. And '.foo' is surely not one of the
> > "known" file extensions.
> 
> You describe a very unusual situation, because prog.foo will not be
> found by the Windows shell and by many other programs that use the
> shell via the likes of 'system' and 'popen'.  I think it's better to
> be 90% correct than do nothing about this issue because we cannot
> easily be 100% correct.  Callers don't usually expect an X_OK test to
> degenerate into F_OK, IMO.
> 
> IOW, I think this implementation doesn't live up to Gnulib's promise
> to be a portability layer, because it loses too much of the baby with
> the bath water.

There are different ways to test for "executable" on Windows:
  - execlp/execvp,
  - CreateProcess,
  - system / popen, like you say,
  - cmd.exe,
  - surely more (PowerShell...)

Let's assume that they work differently (cmd.exe definitely works differently
than execlp/execvp; I tested that).

The module does not attempt to handle all of these, just the first one,
because
  - the function access() is located at the C library level,
  - it is compatible with what the old MSVCRT (without the argument check
    for the mode in _access()) does.

If you consider the gnulib access() function unfit for some purpose, you must
also consider the old MSVCRT _acccess() function unfit for the same purpose.

Bruno

Re: new module 'access'

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: bug-gnulib@gnu.org, psmith@gnu.org
> Date: Sat, 28 Sep 2019 14:06:00 +0200
> 
> There are different ways to test for "executable" on Windows:
>   - execlp/execvp,
>   - CreateProcess,
>   - system / popen, like you say,
>   - cmd.exe,
>   - surely more (PowerShell...)
> 
> Let's assume that they work differently (cmd.exe definitely works differently
> than execlp/execvp; I tested that).
> 
> The module does not attempt to handle all of these, just the first one,
> because
>   - the function access() is located at the C library level,
>   - it is compatible with what the old MSVCRT (without the argument check
>     for the mode in _access()) does.

Well, at the very least how about documenting this subtlety?

> If you consider the gnulib access() function unfit for some purpose, you must
> also consider the old MSVCRT _acccess() function unfit for the same purpose.

Of course I do.  I hope Gnulib doesn't use MSVCRT as an example of a
good implementation in this case ;-)

Re: new module 'access'

[Bruno Haible]

Eli Zaretskii wrote:
> > There are different ways to test for "executable" on Windows:
> >   - execlp/execvp,
> >   - CreateProcess,
> >   - system / popen, like you say,
> >   - cmd.exe,
> >   - surely more (PowerShell...)
> > 
> > Let's assume that they work differently (cmd.exe definitely works differently
> > than execlp/execvp; I tested that).
> > 
> > The module does not attempt to handle all of these, just the first one,
> > because
> >   - the function access() is located at the C library level,
> >   - it is compatible with what the old MSVCRT (without the argument check
> >     for the mode in _access()) does.
> 
> Well, at the very least how about documenting this subtlety?

Done as follows:


2019-09-28  Bruno Haible  <bruno@clisp.org>

	access: Document limitations on Windows.
	Suggested by Zaretskii <eliz@gnu.org>.
	* doc/posix-functions/access.texi: Mention two limitations on Windows.

diff --git a/doc/posix-functions/access.texi b/doc/posix-functions/access.texi
index 49064d2..8bfa2c1 100644
--- a/doc/posix-functions/access.texi
+++ b/doc/posix-functions/access.texi
@@ -31,4 +31,17 @@ this function, especially in a set-uid or set-gid program.
 This function does not have an option for not following symbolic links
 (like @code{stat} versus @code{lstat}).  If you need this option, use
 the Gnulib module @code{faccessat} with the @code{AT_EACCESS} flag.
+@item
+On native Windows, files whose basename does not contain a @samp{.}
+cannot be executed through @code{execlp} or @code{execvp}.  Nevertheless,
+this function may return true for such files.
+@item
+On Windows, different facilities for executing a program have different
+ways of finding an executable file, by trying various suffixes.  For
+example, @code{execlp} and @code{execvp} search for files with the
+suffixes @code{.com}, @code{.exe}, @code{.bat}, @code{.cmd}, when the
+file with the given file name does not exist.  Whereas @code{cmd.exe}
+searches according to the @code{PATHEXT} environment variable.  This
+function does not perform any search; it merely looks at the file with
+the given file name.
 @end itemize

Re: new module 'access'

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: bug-gnulib@gnu.org, psmith@gnu.org
> Date: Sat, 28 Sep 2019 15:29:42 +0200
> 
> > > The module does not attempt to handle all of these, just the first one,
> > > because
> > >   - the function access() is located at the C library level,
> > >   - it is compatible with what the old MSVCRT (without the argument check
> > >     for the mode in _access()) does.
> > 
> > Well, at the very least how about documenting this subtlety?
> 
> Done as follows:
> 
> 
> 2019-09-28  Bruno Haible  <bruno@clisp.org>
> 
> 	access: Document limitations on Windows.
> 	Suggested by Zaretskii <eliz@gnu.org>.
> 	* doc/posix-functions/access.texi: Mention two limitations on Windows.

Thanks!