bug-gnulib@gnu.org mirror (unofficial)
 help / color / mirror / Atom feed
* Z/OS Enhancement to gnu lib
@ 2023-01-30  4:09 Igor Todorovski
  2023-01-30 11:48 ` Bruno Haible
  0 siblings, 1 reply; 4+ messages in thread
From: Igor Todorovski @ 2023-01-30  4:09 UTC (permalink / raw)
  To: bug-gnulib@gnu.org, Mike Fulton

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

Hi,

I was wondering if the following changes I made to findutils (to get it to function on z/OS) can be merged into gnulib.

The first change (fdopendir.c) guards the close call. Otherwise we get a bad file descriptor on z/OS. I am not sure if this has any other consequences, but so far I haven’t seen any issues with findutils.

The second change (openat-proc.c) adds a way to get the pathname when given a file descriptor as an input.

If you have any comments, please let me know:

index c2b0e1e..82ae2e4 100644
--- a/gl/lib/fdopendir.c
+++ b/gl/lib/fdopendir.c
@@ -151,7 +151,9 @@ fdopendir_with_dup (int fd, int older_dupfd, struct saved_cwd const *cwd)
         }
       else
         {
+#ifndef __MVS__
           close (fd);
+#endif
           dir = fd_clone_opendir (dupfd, cwd);
           saved_errno = errno;
           if (! dir)
diff --git a/gl/lib/openat-proc.c b/gl/lib/openat-proc.c
index 3bacf7d..bb788fd 100644
--- a/gl/lib/openat-proc.c
+++ b/gl/lib/openat-proc.c
@@ -28,6 +28,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <termios.h>
#include <unistd.h>
 #ifdef __KLIBC__
@@ -53,7 +54,27 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
       return buf;
     }
-#ifndef __KLIBC__
+#ifdef __MVS__
+  {
+    char dir[_XOPEN_PATH_MAX];
+    int rc = w_ioctl(fd, _IOCC_GPN, _XOPEN_PATH_MAX, dir);
+    if (rc == 0) {
+      __e2a_l(dir, _XOPEN_PATH_MAX);
+    }
+    size_t bufsize;
+    dirlen = strlen (dir);
+    bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */
+    if (OPENAT_BUFFER_SIZE < bufsize)
+      {
+        result = malloc (bufsize);
+        if (! result)
+          return NULL;
+      }
+
+    strcpy (result, dir);
+    result[dirlen++] = '/';
+  }
+#elif !defined( __KLIBC__)
# define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/"
   {
     enum {


[-- Attachment #2: Type: text/html, Size: 14454 bytes --]

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: Z/OS Enhancement to gnu lib
  2023-01-30  4:09 Z/OS Enhancement to gnu lib Igor Todorovski
@ 2023-01-30 11:48 ` Bruno Haible
  2023-01-30 17:29   ` Igor Todorovski
  0 siblings, 1 reply; 4+ messages in thread
From: Bruno Haible @ 2023-01-30 11:48 UTC (permalink / raw)
  To: bug-gnulib, Igor Todorovski; +Cc: Mike Fulton

Hi,

> I was wondering if the following changes I made to findutils (to get it to function on z/OS) can be merged into gnulib.
> 
> The first change (fdopendir.c) guards the close call. Otherwise we get a bad file descriptor on z/OS.

What's the problem with that "bad file descriptor" error? On POSIX system,
EBADF is the natural error code in this situation.

If close() on z/OS does not support this situation gracefully, we'll need
to override the close() function, like we did on native Windows, see
 <https://www.gnu.org/software/gnulib/manual/html_node/close.html>.

> I am not sure if this has any other consequences, but so far I haven’t seen any issues with findutils.

If fd is not invalid, it would lead to a file descriptor leak, no?

> The second change (openat-proc.c) adds a way to get the pathname when given a file descriptor as an input.

Thanks. This looks reasonable.

> diff --git a/gl/lib/openat-proc.c b/gl/lib/openat-proc.c
> index 3bacf7d..bb788fd 100644
> --- a/gl/lib/openat-proc.c
> +++ b/gl/lib/openat-proc.c
> @@ -28,6 +28,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <termios.h>
> #include <unistd.h>
>  #ifdef __KLIBC__
> @@ -53,7 +54,27 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
>        return buf;
>      }
> -#ifndef __KLIBC__
> +#ifdef __MVS__
> +  {
> +    char dir[_XOPEN_PATH_MAX];
> +    int rc = w_ioctl(fd, _IOCC_GPN, _XOPEN_PATH_MAX, dir);
> +    if (rc == 0) {
> +      __e2a_l(dir, _XOPEN_PATH_MAX);
> +    }
> +    size_t bufsize;
> +    dirlen = strlen (dir);
> +    bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */
> +    if (OPENAT_BUFFER_SIZE < bufsize)
> +      {
> +        result = malloc (bufsize);
> +        if (! result)
> +          return NULL;
> +      }
> +
> +    strcpy (result, dir);
> +    result[dirlen++] = '/';
> +  }
> +#elif !defined( __KLIBC__)
> # define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/"
>    {
>      enum {
> 

There are a number of things that can be improved from this patch:
  - The <termios.h> include should only be done on platforms that need it,
    since not all platforms have <termios.h>.
  - GNU coding style: placement of braces, space between function name and
    argument list.
  - Put special cases for special operating systems after the generic/Linux
    case. Simply as a convenience, so that the reader does not turn away
    before getting to the main code.
  - Avoid code duplication of 12 lines of code, if it can easily be avoided.
  - Use 'sizeof dir' instead of '_XOPEN_PATH_MAX', so that the dimension
    of that array needs to be stated only once. Useful for future maintenance.

I did these improvements. What I then take from your patch is only 3 lines
of code. This does not require a copyright assigment from you to the FSF
(cf. <https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html>).
Therefore I am incorporating this code directly.

Btw, do you have reference documentation available for _IOCC_GPN and __e2a_l?
It generally is more advisable to use documented facilities than undocumented
ones.


2023-01-30  Bruno Haible  <bruno@clisp.org>

	at-internal: Add support for z/OS.
	Reported and draft patch by Igor Todorovski <itodorov@ca.ibm.com>.
	* lib/openat-proc.c [z/OS]: Include <termios.h>.
	(openat_proc_name): For z/OS, use an approach similar to kLIBC, with
	3 lines of z/OS specific code by Igor Todorovski <itodorov@ca.ibm.com>.

diff --git a/lib/openat-proc.c b/lib/openat-proc.c
index 2a6a85f069..6419a8cf5f 100644
--- a/lib/openat-proc.c
+++ b/lib/openat-proc.c
@@ -30,9 +30,12 @@
 #include <string.h>
 #include <unistd.h>
 
-#ifdef __KLIBC__
+#ifdef __KLIBC__ /* OS/2 */
 # include <InnoTekLIBC/backend.h>
 #endif
+#ifdef __MVS__ /* z/OS */
+# include <termios.h>
+#endif
 
 #include "intprops.h"
 
@@ -53,7 +56,8 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
       return buf;
     }
 
-#ifndef __KLIBC__
+#if !(defined __KLIBC__ || defined __MVS__)
+  /* Generic code for Linux, Solaris, and similar platforms.  */
 # define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/"
   {
     enum {
@@ -107,14 +111,21 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
         dirlen = sprintf (result, PROC_SELF_FD_FORMAT, fd);
       }
   }
-#else
+#else /* (defined __KLIBC__ || defined __MVS__), i.e. OS/2 or z/OS */
   /* OS/2 kLIBC provides a function to retrieve a path from a fd.  */
   {
-    char dir[_MAX_PATH];
     size_t bufsize;
 
+# ifdef __KLIBC__
+    char dir[_MAX_PATH];
     if (__libc_Back_ioFHToPath (fd, dir, sizeof dir))
       return NULL;
+# endif
+# ifdef __MVS__
+    char dir[_XOPEN_PATH_MAX];
+    if (w_ioctl (fd, _IOCC_GPN, sizeof dir, dir) == 0)
+      __e2a_l (dir, sizeof dir);
+# endif
 
     dirlen = strlen (dir);
     bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */





^ permalink raw reply related	[flat|nested] 4+ messages in thread

* RE: Z/OS Enhancement to gnu lib
  2023-01-30 11:48 ` Bruno Haible
@ 2023-01-30 17:29   ` Igor Todorovski
  2023-01-30 22:12     ` Bruno Haible
  0 siblings, 1 reply; 4+ messages in thread
From: Igor Todorovski @ 2023-01-30 17:29 UTC (permalink / raw)
  To: Bruno Haible, bug-gnulib@gnu.org; +Cc: Mike Fulton

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

This was the symptom we experienced with findutils (guarding the close in gnulib fixed it):
find . -name "test"
The output is:
find: '.': EDC5113I Bad file descriptor.

https://github.com/ZOSOpenTools/findutilsport/issues/4

We’ll do some more testing to see if there’s a better solution. Yes, there’s a chance it will lead to a leak in file descriptors.

__e2a_l is documented here: https://www.ibm.com/docs/en/zos/2.2.0?topic=functions-e2a-l-convert-characters-from-ebcdic-ascii

Unfortunately, _IOCC_GPN is not documented, but is in the exposed header files:
/usr/include/termios.h:      #define _IOCC_GPN           17   /* LFS Get pathname         */

I’ll check with the OS team to see if it can be documented.

Thank you for making the improvements!

Thanks,
Igor

From: Bruno Haible <bruno@clisp.org>
Date: Monday, January 30, 2023 at 6:48 AM
To: bug-gnulib@gnu.org <bug-gnulib@gnu.org>, Igor Todorovski <itodorov@ca.ibm.com>
Cc: Mike Fulton <fultonm@ca.ibm.com>
Subject: [EXTERNAL] Re: Z/OS Enhancement to gnu lib
Hi,

> I was wondering if the following changes I made to findutils (to get it to function on z/OS) can be merged into gnulib.
>
> The first change (fdopendir.c) guards the close call. Otherwise we get a bad file descriptor on z/OS.

What's the problem with that "bad file descriptor" error? On POSIX system,
EBADF is the natural error code in this situation.

If close() on z/OS does not support this situation gracefully, we'll need
to override the close() function, like we did on native Windows, see
 <https://www.gnu.org/software/gnulib/manual/html_node/close.html >.

> I am not sure if this has any other consequences, but so far I haven’t seen any issues with findutils.

If fd is not invalid, it would lead to a file descriptor leak, no?

> The second change (openat-proc.c) adds a way to get the pathname when given a file descriptor as an input.

Thanks. This looks reasonable.

> diff --git a/gl/lib/openat-proc.c b/gl/lib/openat-proc.c
> index 3bacf7d..bb788fd 100644
> --- a/gl/lib/openat-proc.c
> +++ b/gl/lib/openat-proc.c
> @@ -28,6 +28,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <termios.h>
> #include <unistd.h>
>  #ifdef __KLIBC__
> @@ -53,7 +54,27 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
>        return buf;
>      }
> -#ifndef __KLIBC__
> +#ifdef __MVS__
> +  {
> +    char dir[_XOPEN_PATH_MAX];
> +    int rc = w_ioctl(fd, _IOCC_GPN, _XOPEN_PATH_MAX, dir);
> +    if (rc == 0) {
> +      __e2a_l(dir, _XOPEN_PATH_MAX);
> +    }
> +    size_t bufsize;
> +    dirlen = strlen (dir);
> +    bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */
> +    if (OPENAT_BUFFER_SIZE < bufsize)
> +      {
> +        result = malloc (bufsize);
> +        if (! result)
> +          return NULL;
> +      }
> +
> +    strcpy (result, dir);
> +    result[dirlen++] = '/';
> +  }
> +#elif !defined( __KLIBC__)
> # define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/"
>    {
>      enum {
>

There are a number of things that can be improved from this patch:
  - The <termios.h> include should only be done on platforms that need it,
    since not all platforms have <termios.h>.
  - GNU coding style: placement of braces, space between function name and
    argument list.
  - Put special cases for special operating systems after the generic/Linux
    case. Simply as a convenience, so that the reader does not turn away
    before getting to the main code.
  - Avoid code duplication of 12 lines of code, if it can easily be avoided.
  - Use 'sizeof dir' instead of '_XOPEN_PATH_MAX', so that the dimension
    of that array needs to be stated only once. Useful for future maintenance.

I did these improvements. What I then take from your patch is only 3 lines
of code. This does not require a copyright assigment from you to the FSF
(cf. <https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html >).
Therefore I am incorporating this code directly.

Btw, do you have reference documentation available for _IOCC_GPN and __e2a_l?
It generally is more advisable to use documented facilities than undocumented
ones.


2023-01-30  Bruno Haible  <bruno@clisp.org>

        at-internal: Add support for z/OS.
        Reported and draft patch by Igor Todorovski <itodorov@ca.ibm.com>.
        * lib/openat-proc.c [z/OS]: Include <termios.h>.
        (openat_proc_name): For z/OS, use an approach similar to kLIBC, with
        3 lines of z/OS specific code by Igor Todorovski <itodorov@ca.ibm.com>.

diff --git a/lib/openat-proc.c b/lib/openat-proc.c
index 2a6a85f069..6419a8cf5f 100644
--- a/lib/openat-proc.c
+++ b/lib/openat-proc.c
@@ -30,9 +30,12 @@
 #include <string.h>
 #include <unistd.h>

-#ifdef __KLIBC__
+#ifdef __KLIBC__ /* OS/2 */
 # include <InnoTekLIBC/backend.h>
 #endif
+#ifdef __MVS__ /* z/OS */
+# include <termios.h>
+#endif

 #include "intprops.h"

@@ -53,7 +56,8 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
       return buf;
     }

-#ifndef __KLIBC__
+#if !(defined __KLIBC__ || defined __MVS__)
+  /* Generic code for Linux, Solaris, and similar platforms.  */
 # define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/"
   {
     enum {
@@ -107,14 +111,21 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
         dirlen = sprintf (result, PROC_SELF_FD_FORMAT, fd);
       }
   }
-#else
+#else /* (defined __KLIBC__ || defined __MVS__), i.e. OS/2 or z/OS */
   /* OS/2 kLIBC provides a function to retrieve a path from a fd.  */
   {
-    char dir[_MAX_PATH];
     size_t bufsize;

+# ifdef __KLIBC__
+    char dir[_MAX_PATH];
     if (__libc_Back_ioFHToPath (fd, dir, sizeof dir))
       return NULL;
+# endif
+# ifdef __MVS__
+    char dir[_XOPEN_PATH_MAX];
+    if (w_ioctl (fd, _IOCC_GPN, sizeof dir, dir) == 0)
+      __e2a_l (dir, sizeof dir);
+# endif

     dirlen = strlen (dir);
     bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */



[-- Attachment #2: Type: text/html, Size: 12361 bytes --]

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: Z/OS Enhancement to gnu lib
  2023-01-30 17:29   ` Igor Todorovski
@ 2023-01-30 22:12     ` Bruno Haible
  0 siblings, 0 replies; 4+ messages in thread
From: Bruno Haible @ 2023-01-30 22:12 UTC (permalink / raw)
  To: bug-gnulib, Igor Todorovski; +Cc: Mike Fulton

Igor Todorovski wrote:
> This was the symptom we experienced with findutils (guarding the close in gnulib fixed it):
> find . -name "test"
> The output is:
> find: '.': EDC5113I Bad file descriptor.
> 
> https://github.com/ZOSOpenTools/findutilsport/issues/4
> 
> We’ll do some more testing to see if there’s a better solution.

I'm sending you a testdir, generated through

  ./gnulib-tool --create-testdir --dir=/tmp/testdir --single-configure \
                areadlinkat areadlinkat-with-size faccessat fchmodat fchownat 
                fdopendir getcwd fstatat linkat mkdirat mkfifoat openat 
                readlinkat renameatu savedir symlinkat unlinkat utimensat

As part of your testing, please run it through
  ./configure && make && make check

(Sent via private mail, as it's too large for this mailing list.)

> __e2a_l is documented here: https://www.ibm.com/docs/en/zos/2.2.0?topic=functions-e2a-l-convert-characters-from-ebcdic-ascii

Well, according to this documentation your patch was buggy: it did not
check the return value. Additionally, it ran a conversion on a buffer
full of uninitialized data (which is 1. slow and 2. triggers warnings
in tools like valgrind).  And when w_ioctl failed, your code was
accessing and returning unitialized data in dir[]. Ouch ouch ouch.

Fixing it like this:


2023-01-30  Bruno Haible  <bruno@clisp.org>

	at-internal: Fix support for z/OS.
	* lib/openat-proc.c (openat_proc_name) [z/OS]: Proper error handling.
	Convert only the relevant part of the dir[] buffer.

diff --git a/lib/openat-proc.c b/lib/openat-proc.c
index 6419a8cf5f..88f70be4f5 100644
--- a/lib/openat-proc.c
+++ b/lib/openat-proc.c
@@ -123,8 +123,16 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file)
 # endif
 # ifdef __MVS__
     char dir[_XOPEN_PATH_MAX];
-    if (w_ioctl (fd, _IOCC_GPN, sizeof dir, dir) == 0)
-      __e2a_l (dir, sizeof dir);
+    /* Documentation:
+       https://www.ibm.com/docs/en/zos/2.2.0?topic=functions-w-ioctl-w-pioctl-control-devices */
+    if (w_ioctl (fd, _IOCC_GPN, sizeof dir, dir) < 0)
+      return NULL;
+    /* Documentation:
+       https://www.ibm.com/docs/en/zos/2.2.0?topic=functions-e2a-l-convert-characters-from-ebcdic-ascii */
+    dirlen = __e2a_l (dir, strlen (dir));
+    if (dirlen < 0 || dirlen >= sizeof dir)
+      return NULL;
+    dir[dirlen] = '\0';
 # endif
 
     dirlen = strlen (dir);





^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-01-30 22:12 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-30  4:09 Z/OS Enhancement to gnu lib Igor Todorovski
2023-01-30 11:48 ` Bruno Haible
2023-01-30 17:29   ` Igor Todorovski
2023-01-30 22:12     ` Bruno Haible

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).