Re: Better error output for readlink(1)

[Scott Cheloha]

> On Apr 5, 2017, at 2:48 AM, Nicholas Marriott wrote:
> 
>> On Tue, Apr 04, 2017 at 05:03:26PM -0500, Scott Cheloha wrote:
>> 
>> [...]
>> 
>> In the current code, however, you could have insufficient permissions
>> for a part of the path (EPERM), or an I/O failure (EIO), but otherwise
>> specify a valid symbolic link, and still get the described behavior,
>> which seems wrong to me.
>> 
>> If the documented "say nothing and exit 1" behavior when the target
>> is not a symbolic link is sacred, then maybe this snippet is better:
> 
> There are likely to be existing scripts that rely on this behaviour.

The EPERM/EIO silent failure?  Okay.

> [...]
> 
> I think it would be better not to change the behaviour, except possibly
> for ENAMETOOLONG.

Agreed.  Adjusted proposal:

Eliminate the strlen(3) call because both realpath(3) and readlink(2) do
length checking anyway.  Leverage err(3)/warn(3) and the error string for
ENAMETOOLONG to report when this happens in lieu of the custom error
message used now.

Aside from the format of the error message when the file name argument
exceeds PATH_MAX bytes, the only change in behavior is that readlink(1)
*without* the -f flag now complains about the file name being too long
if a portion of the path name exceeds NAME_MAX bytes.  To see this,
compare

readlink $(perl -e 'print "z"x256')

with/without the patch.

While here, make some other style(9)-type changes.

Index: readlink.c
===
RCS file: /cvs/src/usr.bin/readlink/readlink.c,v
retrieving revision 1.27
diff -u -p -r1.27 readlink.c
--- readlink.c  9 Oct 2015 01:37:08 -   1.27
+++ readlink.c  6 Apr 2017 00:13:34 -
@@ -32,17 +32,17 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
-static voidusage(void);
+static __dead void usage(void);
 
 int
 main(int argc, char *argv[])
 {
char buf[PATH_MAX];
-   int n, ch, nflag = 0, fflag = 0;
-   extern int optind;
+   int ch, fflag, n, nflag;
+
+   fflag = nflag = 0;
 
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
@@ -64,30 +64,25 @@ main(int argc, char *argv[])
if (argc != 1)
usage();
 
-   n = strlen(argv[0]);
-   if (n > PATH_MAX - 1) {
-   fprintf(stderr,
-   "readlink: filename longer than PATH_MAX-1 (%d)\n",
-   PATH_MAX - 1);
-   exit(1);
-   }
-
if (fflag) {
if (realpath(argv[0], buf) == NULL)
err(1, "%s", argv[0]);
} else {
-   if ((n = readlink(argv[0], buf, sizeof buf-1)) < 0)
-   exit(1);
+   if ((n = readlink(argv[0], buf, sizeof(buf) - 1)) == -1) {
+   if (errno == ENAMETOOLONG)
+   warn("%s", argv[0]);
+   return 1;
+   }
buf[n] = '\0';
}
 
printf("%s", buf);
if (!nflag)
putchar('\n');
-   exit(0);
+   return 0;
 }
 
-static void
+static __dead void
 usage(void)
 {
(void)fprintf(stderr, "usage: readlink [-fn] file\n");

Re: Better error output for readlink(1)

[Nicholas Marriott]

On Tue, Apr 04, 2017 at 05:03:26PM -0500, Scott Cheloha wrote:
> > On Apr 4, 2017, at 4:46 PM, Nicholas Marriott  
> > wrote:
> > 
> > readlink is explicitly documented to silently exit 1 if run without -f,
> > and GNU readlink behaves the same way. I doubt that should change.
> 
> Yeah, I saw that.  I (incorrectly, I guess?) interpreted it to mean
> "does not print anything on the standard output."
> 
> My think is -- and maybe I'm being nitpicky, but -- the documentation
> says:
> 
> > If the -f option is not specified and readlink is invoked with an
> > argument other than the pathname of a symbolic link, it exits with
> > a nonzero exit code without printing anything.
> 
> In the current code, however, you could have insufficient permissions
> for a part of the path (EPERM), or an I/O failure (EIO), but otherwise
> specify a valid symbolic link, and still get the described behavior,
> which seems wrong to me.
> 
> If the documented "say nothing and exit 1" behavior when the target
> is not a symbolic link is sacred, then maybe this snippet is better:

There are likely to be existing scripts that rely on this behaviour.

> 
>   if (fflag) {
>   if (realpath(argv[0], buf) == NULL)
>   err(1, "%s", argv[0]);
>   } else {
> - if ((n = readlink(argv[0], buf, sizeof buf-1)) < 0)
> - exit(1);
> + if ((n = readlink(argv[0], buf, sizeof(buf) - 1)) == -1) {
> + if (errno != EINVAL)
> + warn("%s", argv[0]);

You will definitely also need to check for at least ENOENT and ENOTDIR,
possibly others too.

I think it would be better not to change the behaviour, except possibly
for ENAMETOOLONG.

> + return 1;
> + }
>   buf[n] = '\0';
>   }
> 
> That way you still say nothing if the issue is, in fact, that your target
> isn't a symbolic link, but you point out any other errors.
> 
> --
> Scott Cheloha

Re: Better error output for readlink(1)

[Scott Cheloha]

> On Apr 4, 2017, at 4:46 PM, Nicholas Marriott  
> wrote:
> 
> readlink is explicitly documented to silently exit 1 if run without -f,
> and GNU readlink behaves the same way. I doubt that should change.

Yeah, I saw that.  I (incorrectly, I guess?) interpreted it to mean
"does not print anything on the standard output."

My think is -- and maybe I'm being nitpicky, but -- the documentation
says:

> If the -f option is not specified and readlink is invoked with an
> argument other than the pathname of a symbolic link, it exits with
> a nonzero exit code without printing anything.

In the current code, however, you could have insufficient permissions
for a part of the path (EPERM), or an I/O failure (EIO), but otherwise
specify a valid symbolic link, and still get the described behavior,
which seems wrong to me.

If the documented "say nothing and exit 1" behavior when the target
is not a symbolic link is sacred, then maybe this snippet is better:

if (fflag) {
if (realpath(argv[0], buf) == NULL)
err(1, "%s", argv[0]);
} else {
-   if ((n = readlink(argv[0], buf, sizeof buf-1)) < 0)
-   exit(1);
+   if ((n = readlink(argv[0], buf, sizeof(buf) - 1)) == -1) {
+   if (errno != EINVAL)
+   warn("%s", argv[0]);
+   return 1;
+   }
buf[n] = '\0';
}

That way you still say nothing if the issue is, in fact, that your target
isn't a symbolic link, but you point out any other errors.

--
Scott Cheloha

Re: Better error output for readlink(1)

[Nicholas Marriott]

Hi

readlink is explicitly documented to silently exit 1 if run without -f,
and GNU readlink behaves the same way. I doubt that should change.



On Tue, Apr 04, 2017 at 04:40:19PM -0500, Scott Cheloha wrote:
> This patch replaces a custom error message in readlink.c with err(3).
> The custom message and call to strlen(3) aren't needed because
> readlink(2) checks the length of the argument string and sets errno
> to ENAMETOOLONG if it is too long.
> 
> Dropping strlen(3) lets us drop string.h.
> 
> Using err(3) also lets us trivially report the myriad ways readlink(2)
> can fail.  At the moment we just exit 1, which can be misleading during
> interactive use.
> 
> While here, do other miscellaneous style(9)-type changes.
> 
> Any takers?
> 
> --
> Scott Cheloha
> 
> P.S. returning from main() is preferred over exit(3), right?
> 
> Index: readlink.c
> ===
> RCS file: /cvs/src/usr.bin/readlink/readlink.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 readlink.c
> --- readlink.c9 Oct 2015 01:37:08 -   1.27
> +++ readlink.c4 Apr 2017 21:10:41 -
> @@ -32,17 +32,17 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  
> -static void  usage(void);
> +static __dead void   usage(void);
>  
>  int
>  main(int argc, char *argv[])
>  {
>   char buf[PATH_MAX];
> - int n, ch, nflag = 0, fflag = 0;
> - extern int optind;
> + int ch, fflag, n, nflag;
> +
> + fflag = nflag = 0;
>  
>   if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
> @@ -64,30 +64,26 @@ main(int argc, char *argv[])
>   if (argc != 1)
>   usage();
>  
> - n = strlen(argv[0]);
> - if (n > PATH_MAX - 1) {
> - fprintf(stderr,
> - "readlink: filename longer than PATH_MAX-1 (%d)\n",
> - PATH_MAX - 1);
> - exit(1);
> - }
> -
>   if (fflag) {
>   if (realpath(argv[0], buf) == NULL)
>   err(1, "%s", argv[0]);
>   } else {
> - if ((n = readlink(argv[0], buf, sizeof buf-1)) < 0)
> - exit(1);
> + if ((n = readlink(argv[0], buf, sizeof(buf) - 1)) == -1) {
> + if (errno == EINVAL)
> + errx(1, "%s: Not a symbolic link", argv[0]);
> + else
> + err(1, "%s", argv[0]);
> + }
>   buf[n] = '\0';
>   }
>  
>   printf("%s", buf);
>   if (!nflag)
>   putchar('\n');
> - exit(0);
> + return 0;
>  }
>  
> -static void
> +static __dead void
>  usage(void)
>  {
>   (void)fprintf(stderr, "usage: readlink [-fn] file\n");
> 

Better error output for readlink(1)

[Scott Cheloha]

This patch replaces a custom error message in readlink.c with err(3).
The custom message and call to strlen(3) aren't needed because
readlink(2) checks the length of the argument string and sets errno
to ENAMETOOLONG if it is too long.

Dropping strlen(3) lets us drop string.h.

Using err(3) also lets us trivially report the myriad ways readlink(2)
can fail.  At the moment we just exit 1, which can be misleading during
interactive use.

While here, do other miscellaneous style(9)-type changes.

Any takers?

--
Scott Cheloha

P.S. returning from main() is preferred over exit(3), right?

Index: readlink.c
===
RCS file: /cvs/src/usr.bin/readlink/readlink.c,v
retrieving revision 1.27
diff -u -p -r1.27 readlink.c
--- readlink.c  9 Oct 2015 01:37:08 -   1.27
+++ readlink.c  4 Apr 2017 21:10:41 -
@@ -32,17 +32,17 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
-static voidusage(void);
+static __dead void usage(void);
 
 int
 main(int argc, char *argv[])
 {
char buf[PATH_MAX];
-   int n, ch, nflag = 0, fflag = 0;
-   extern int optind;
+   int ch, fflag, n, nflag;
+
+   fflag = nflag = 0;
 
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
@@ -64,30 +64,26 @@ main(int argc, char *argv[])
if (argc != 1)
usage();
 
-   n = strlen(argv[0]);
-   if (n > PATH_MAX - 1) {
-   fprintf(stderr,
-   "readlink: filename longer than PATH_MAX-1 (%d)\n",
-   PATH_MAX - 1);
-   exit(1);
-   }
-
if (fflag) {
if (realpath(argv[0], buf) == NULL)
err(1, "%s", argv[0]);
} else {
-   if ((n = readlink(argv[0], buf, sizeof buf-1)) < 0)
-   exit(1);
+   if ((n = readlink(argv[0], buf, sizeof(buf) - 1)) == -1) {
+   if (errno == EINVAL)
+   errx(1, "%s: Not a symbolic link", argv[0]);
+   else
+   err(1, "%s", argv[0]);
+   }
buf[n] = '\0';
}
 
printf("%s", buf);
if (!nflag)
putchar('\n');
-   exit(0);
+   return 0;
 }
 
-static void
+static __dead void
 usage(void)
 {
(void)fprintf(stderr, "usage: readlink [-fn] file\n");