Re: read(2) on directories

[Jeremie Courreges-Anglas]

"Theo de Raadt"  writes:

>> On 2016-09-26, Jeremie Courreges-Anglas  wrote:
>> 
>> >>> So I think that we agree that EISDIR is more useful, and seems safe from
>> >>> a portability POV.   I've built base and x sets on i386, and ajacoutot
>> >>> ran the ports bulk builds.  The two offenders in the ports tree were due
>> >>> to an unrelated glitch in base libtool which has since been fixed.
>> >
>> > I haven't received a single test report, which is far from sufficient
>> > for such a change.  Even though I'm convinced that such a change would
>> > be a benefit, I won't push this further.
>> 
>> I think your proposal came at a bad time when there was too much
>> other action in the tree.
>> 
>> I've run an amd64 package build with it (because I didn't read the
>> above that said that aja had already done so), which worked just
>> fine.  I don't think we're going to see more of a real-world test
>> unless the diff goes into snapshots.
>> 
>> FWIW, I'm in favor of this change.
>
> Indeed, and the timing is much better.  Let's do it, and keep an
> eye out for fallout.

Committed.  Please let me know if it produces unexpected, nasty side
effects.

Thanks folks,
-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: read(2) on directories

[Theo de Raadt]

> On 2016-09-26, Jeremie Courreges-Anglas  wrote:
> 
> >>> So I think that we agree that EISDIR is more useful, and seems safe from
> >>> a portability POV.   I've built base and x sets on i386, and ajacoutot
> >>> ran the ports bulk builds.  The two offenders in the ports tree were due
> >>> to an unrelated glitch in base libtool which has since been fixed.
> >
> > I haven't received a single test report, which is far from sufficient
> > for such a change.  Even though I'm convinced that such a change would
> > be a benefit, I won't push this further.
> 
> I think your proposal came at a bad time when there was too much
> other action in the tree.
> 
> I've run an amd64 package build with it (because I didn't read the
> above that said that aja had already done so), which worked just
> fine.  I don't think we're going to see more of a real-world test
> unless the diff goes into snapshots.
> 
> FWIW, I'm in favor of this change.

Indeed, and the timing is much better.  Let's do it, and keep an
eye out for fallout.

Re: read(2) on directories

[Christian Weisgerber]

On 2016-09-26, Jeremie Courreges-Anglas  wrote:

>>> So I think that we agree that EISDIR is more useful, and seems safe from
>>> a portability POV.   I've built base and x sets on i386, and ajacoutot
>>> ran the ports bulk builds.  The two offenders in the ports tree were due
>>> to an unrelated glitch in base libtool which has since been fixed.
>
> I haven't received a single test report, which is far from sufficient
> for such a change.  Even though I'm convinced that such a change would
> be a benefit, I won't push this further.

I think your proposal came at a bad time when there was too much
other action in the tree.

I've run an amd64 package build with it (because I didn't read the
above that said that aja had already done so), which worked just
fine.  I don't think we're going to see more of a real-world test
unless the diff goes into snapshots.

FWIW, I'm in favor of this change.

-- 
Christian "naddy" Weisgerber  na...@mips.inka.de

Re: read(2) on directories

[Alexander Bluhm]

On Mon, Sep 26, 2016 at 06:14:10PM +0200, Jeremie Courreges-Anglas wrote:
> I haven't received a single test report, which is far from sufficient
> for such a change.  Even though I'm convinced that such a change would
> be a benefit, I won't push this further.

I am running this diff on my laptop since Aug 8.  As everything
works fine, I have totally forgotten to report the result.  Sorry.

I liked this behavior when I was using BSD/OS decades ago.

The error = 0 initialization is not necessary anymore with you diff.

OK bluhm, FWIW

> > Diff below for convenience.
> >
> >
> > Index: lib/libc/sys/read.2
> > ===
> > RCS file: /cvs/src/lib/libc/sys/read.2,v
> > retrieving revision 1.35
> > diff -u -p -r1.35 read.2
> > --- lib/libc/sys/read.2 5 Feb 2015 02:33:09 -   1.35
> > +++ lib/libc/sys/read.2 9 Jul 2016 17:20:39 -
> > @@ -152,13 +152,15 @@ is not a valid file or socket descriptor
> >  Part of
> >  .Fa buf
> >  points outside the process's allocated address space.
> > -.It Bq Er EIO
> > -An I/O error occurred while reading from the file system.
> >  .It Bq Er EINTR
> >  A read from a slow device
> >  (i.e. one that might block for an arbitrary amount of time)
> >  was interrupted by the delivery of a signal
> >  before any data arrived.
> > +.It Bq Er EIO
> > +An I/O error occurred while reading from the file system.
> > +.It Bq Er EISDIR
> > +The underlying file is a directory.
> >  .El
> >  .Pp
> >  In addition,
> > Index: sys/kern/vfs_vnops.c
> > ===
> > RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
> > retrieving revision 1.85
> > diff -u -p -r1.85 vfs_vnops.c
> > --- sys/kern/vfs_vnops.c19 Jun 2016 11:54:33 -  1.85
> > +++ sys/kern/vfs_vnops.c9 Jul 2016 17:20:39 -
> > @@ -336,11 +336,13 @@ vn_read(struct file *fp, off_t *poff, st
> > if (vp->v_type != VCHR && count > LLONG_MAX - *poff)
> > return (EINVAL);
> >  
> > +   if (vp->v_type == VDIR)
> > +   return (EISDIR);
> > +
> > vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
> > uio->uio_offset = *poff;
> > -   if (vp->v_type != VDIR)
> > -   error = VOP_READ(vp, uio,
> > -   (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0, cred);
> > +   error = VOP_READ(vp, uio, (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0,
> > +   cred);
> > *poff += count - uio->uio_resid;
> > VOP_UNLOCK(vp, p);
> > return (error);
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: read(2) on directories

[Jeremie Courreges-Anglas]

j...@wxcvbn.org (Jeremie Courreges-Anglas) writes:

> j...@wxcvbn.org (Jeremie Courreges-Anglas) writes:
>
>> "Todd C. Miller"  writes:
>>
>>> From source inspection, Net and Free appear to allow read(2) of
>>> dirs to succeed.  However, since Linux, Mac OS X and Solaris have
>>> the EISDIR behavior I think it is probably safe from a portability
>>> standpoint.
>>>
>>> We're long past the days when opendir(3)/readdir(3) used read(2)...
>>>
>>> HP-UX and AIX still allow reads of directories but no one cares
>>> about them ;-)
>>
>> So I think that we agree that EISDIR is more useful, and seems safe from
>> a portability POV.   I've built base and x sets on i386, and ajacoutot
>> ran the ports bulk builds.  The two offenders in the ports tree were due
>> to an unrelated glitch in base libtool which has since been fixed.
>
> I've hold this for a few days until the release cycle comes back to an
> almost normal state.  But, as Theo points out, we have to think about the
> drawbacks.  Some configurations that do work right now are very likely
> to break.
>
>   $ cat /; echo $?
>   cat: /: Is a directory
>   1
>
> is only a trivial example.
>
> I still think that it is better to error out instead of, for example,
> silently ignoring invalid/mistyped CLI arguments and config parameters.
> But I don't want to mindlessly break people's setups behind their backs,
> so I'd like to hear more opinions and... *test reports*.

I haven't received a single test report, which is far from sufficient
for such a change.  Even though I'm convinced that such a change would
be a benefit, I won't push this further.

> Diff below for convenience.
>
>
> Index: lib/libc/sys/read.2
> ===
> RCS file: /cvs/src/lib/libc/sys/read.2,v
> retrieving revision 1.35
> diff -u -p -r1.35 read.2
> --- lib/libc/sys/read.2   5 Feb 2015 02:33:09 -   1.35
> +++ lib/libc/sys/read.2   9 Jul 2016 17:20:39 -
> @@ -152,13 +152,15 @@ is not a valid file or socket descriptor
>  Part of
>  .Fa buf
>  points outside the process's allocated address space.
> -.It Bq Er EIO
> -An I/O error occurred while reading from the file system.
>  .It Bq Er EINTR
>  A read from a slow device
>  (i.e. one that might block for an arbitrary amount of time)
>  was interrupted by the delivery of a signal
>  before any data arrived.
> +.It Bq Er EIO
> +An I/O error occurred while reading from the file system.
> +.It Bq Er EISDIR
> +The underlying file is a directory.
>  .El
>  .Pp
>  In addition,
> Index: sys/kern/vfs_vnops.c
> ===
> RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 vfs_vnops.c
> --- sys/kern/vfs_vnops.c  19 Jun 2016 11:54:33 -  1.85
> +++ sys/kern/vfs_vnops.c  9 Jul 2016 17:20:39 -
> @@ -336,11 +336,13 @@ vn_read(struct file *fp, off_t *poff, st
>   if (vp->v_type != VCHR && count > LLONG_MAX - *poff)
>   return (EINVAL);
>  
> + if (vp->v_type == VDIR)
> + return (EISDIR);
> +
>   vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
>   uio->uio_offset = *poff;
> - if (vp->v_type != VDIR)
> - error = VOP_READ(vp, uio,
> - (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0, cred);
> + error = VOP_READ(vp, uio, (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0,
> + cred);
>   *poff += count - uio->uio_resid;
>   VOP_UNLOCK(vp, p);
>   return (error);

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: read(2) on directories

[Theo de Raadt]

>> From source inspection, Net and Free appear to allow read(2) of
>> dirs to succeed.  However, since Linux, Mac OS X and Solaris have
>> the EISDIR behavior I think it is probably safe from a portability
>> standpoint.

I want to explain why I chose the semantic of "read returns 0",
about 20 years ago I guess.

All of those systems already have that semantic on NFS directories --
directory fd reads return 0.

Therefore, it seemed reasonable to assume that all programs could
already handle that case properly.  Sun and CSRG had done the auditing
work on the base utilities during the 90's, so applying the same
rule to native filesystem seemed sound.

It was simply impossible to add a new error condition to system call
#4, without auditing everything.

Maybe some of these vendors continued their utility audit and switched
to EISDIR on native filesystems.  Well we haven't done that work
yet.

Re: read(2) on directories

[Theo de Raadt]

> > "Todd C. Miller"  writes:
> >
> >> From source inspection, Net and Free appear to allow read(2) of
> >> dirs to succeed.  However, since Linux, Mac OS X and Solaris have
> >> the EISDIR behavior I think it is probably safe from a portability
> >> standpoint.
> >>
> >> We're long past the days when opendir(3)/readdir(3) used read(2)...
> >>
> >> HP-UX and AIX still allow reads of directories but no one cares
> >> about them ;-)
> >
> > So I think that we agree that EISDIR is more useful, and seems safe from
> > a portability POV.   I've built base and x sets on i386, and ajacoutot
> > ran the ports bulk builds.  The two offenders in the ports tree were due
> > to an unrelated glitch in base libtool which has since been fixed.
> 
> I've hold this for a few days until the release cycle comes back to an
> almost normal state.  But, as Theo points out, we have to think about the
> drawbacks.  Some configurations that do work right now are very likely
> to break.
> 
>   $ cat /; echo $?
>   cat: /: Is a directory
>   1
> 
> is only a trivial example.
> 
> I still think that it is better to error out instead of, for example,
> silently ignoring invalid/mistyped CLI arguments and config parameters.
> But I don't want to mindlessly break people's setups behind their backs,
> so I'd like to hear more opinions and... *test reports*.

The change will affect a huge number of program in significant ways.
I am waiting for people to actually try itd, and I haven't been seeing
that.

I don't like the cat example above.  It does not actually expose the
full change in behaviour from this simple program.  Here is a better
example:

   % cat / /etc/shells; echo $?
   #   $OpenBSD: shells,v 1.8 2009/02/14 17:06:40 sobrado Exp $
   #
   # list of acceptable shells for chpass(1).
   # ftpd(8) will not allow users to connect who are not using
   # one of these shells, unless the user is listed in /etc/ftpchroot.
   /bin/sh
   /bin/csh
   /bin/ksh
   0
   %

Will become:

   % cat / /etc/shells; echo $?
   cat: /: Is a directory
   1
   %

The semantic will cause a subtle behavioural difference in hundreds
of programs.

Re: read(2) on directories

[Jeremie Courreges-Anglas]

j...@wxcvbn.org (Jeremie Courreges-Anglas) writes:

> "Todd C. Miller"  writes:
>
>> From source inspection, Net and Free appear to allow read(2) of
>> dirs to succeed.  However, since Linux, Mac OS X and Solaris have
>> the EISDIR behavior I think it is probably safe from a portability
>> standpoint.
>>
>> We're long past the days when opendir(3)/readdir(3) used read(2)...
>>
>> HP-UX and AIX still allow reads of directories but no one cares
>> about them ;-)
>
> So I think that we agree that EISDIR is more useful, and seems safe from
> a portability POV.   I've built base and x sets on i386, and ajacoutot
> ran the ports bulk builds.  The two offenders in the ports tree were due
> to an unrelated glitch in base libtool which has since been fixed.

I've hold this for a few days until the release cycle comes back to an
almost normal state.  But, as Theo points out, we have to think about the
drawbacks.  Some configurations that do work right now are very likely
to break.

  $ cat /; echo $?
  cat: /: Is a directory
  1

is only a trivial example.

I still think that it is better to error out instead of, for example,
silently ignoring invalid/mistyped CLI arguments and config parameters.
But I don't want to mindlessly break people's setups behind their backs,
so I'd like to hear more opinions and... *test reports*.

Diff below for convenience.


Index: lib/libc/sys/read.2
===
RCS file: /cvs/src/lib/libc/sys/read.2,v
retrieving revision 1.35
diff -u -p -r1.35 read.2
--- lib/libc/sys/read.2 5 Feb 2015 02:33:09 -   1.35
+++ lib/libc/sys/read.2 9 Jul 2016 17:20:39 -
@@ -152,13 +152,15 @@ is not a valid file or socket descriptor
 Part of
 .Fa buf
 points outside the process's allocated address space.
-.It Bq Er EIO
-An I/O error occurred while reading from the file system.
 .It Bq Er EINTR
 A read from a slow device
 (i.e. one that might block for an arbitrary amount of time)
 was interrupted by the delivery of a signal
 before any data arrived.
+.It Bq Er EIO
+An I/O error occurred while reading from the file system.
+.It Bq Er EISDIR
+The underlying file is a directory.
 .El
 .Pp
 In addition,
Index: sys/kern/vfs_vnops.c
===
RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
retrieving revision 1.85
diff -u -p -r1.85 vfs_vnops.c
--- sys/kern/vfs_vnops.c19 Jun 2016 11:54:33 -  1.85
+++ sys/kern/vfs_vnops.c9 Jul 2016 17:20:39 -
@@ -336,11 +336,13 @@ vn_read(struct file *fp, off_t *poff, st
if (vp->v_type != VCHR && count > LLONG_MAX - *poff)
return (EINVAL);
 
+   if (vp->v_type == VDIR)
+   return (EISDIR);
+
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
uio->uio_offset = *poff;
-   if (vp->v_type != VDIR)
-   error = VOP_READ(vp, uio,
-   (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0, cred);
+   error = VOP_READ(vp, uio, (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0,
+   cred);
*poff += count - uio->uio_resid;
VOP_UNLOCK(vp, p);
return (error);


-- 
jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: read(2) on directories

[Todd C. Miller]

On Tue, 02 Aug 2016 18:42:43 +0200, Jeremie Courreges-Anglas wrote:

> So I think that we agree that EISDIR is more useful, and seems safe from
> a portability POV.   I've built base and x sets on i386, and ajacoutot
> ran the ports bulk builds.  The two offenders in the ports tree were due
> to an unrelated glitch in base libtool which has since been fixed.

OK millert@

 - todd

Re: read(2) on directories

[Jeremie Courreges-Anglas]

"Todd C. Miller"  writes:

> From source inspection, Net and Free appear to allow read(2) of
> dirs to succeed.  However, since Linux, Mac OS X and Solaris have
> the EISDIR behavior I think it is probably safe from a portability
> standpoint.
>
> We're long past the days when opendir(3)/readdir(3) used read(2)...
>
> HP-UX and AIX still allow reads of directories but no one cares
> about them ;-)

So I think that we agree that EISDIR is more useful, and seems safe from
a portability POV.   I've built base and x sets on i386, and ajacoutot
ran the ports bulk builds.  The two offenders in the ports tree were due
to an unrelated glitch in base libtool which has since been fixed.

ok?


Index: lib/libc/sys/read.2
===
RCS file: /cvs/src/lib/libc/sys/read.2,v
retrieving revision 1.35
diff -u -p -r1.35 read.2
--- lib/libc/sys/read.2 5 Feb 2015 02:33:09 -   1.35
+++ lib/libc/sys/read.2 9 Jul 2016 17:20:39 -
@@ -152,13 +152,15 @@ is not a valid file or socket descriptor
 Part of
 .Fa buf
 points outside the process's allocated address space.
-.It Bq Er EIO
-An I/O error occurred while reading from the file system.
 .It Bq Er EINTR
 A read from a slow device
 (i.e. one that might block for an arbitrary amount of time)
 was interrupted by the delivery of a signal
 before any data arrived.
+.It Bq Er EIO
+An I/O error occurred while reading from the file system.
+.It Bq Er EISDIR
+The underlying file is a directory.
 .El
 .Pp
 In addition,
Index: sys/kern/vfs_vnops.c
===
RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
retrieving revision 1.85
diff -u -p -r1.85 vfs_vnops.c
--- sys/kern/vfs_vnops.c19 Jun 2016 11:54:33 -  1.85
+++ sys/kern/vfs_vnops.c9 Jul 2016 17:20:39 -
@@ -336,11 +336,13 @@ vn_read(struct file *fp, off_t *poff, st
if (vp->v_type != VCHR && count > LLONG_MAX - *poff)
return (EINVAL);
 
+   if (vp->v_type == VDIR)
+   return (EISDIR);
+
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
uio->uio_offset = *poff;
-   if (vp->v_type != VDIR)
-   error = VOP_READ(vp, uio,
-   (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0, cred);
+   error = VOP_READ(vp, uio, (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0,
+   cred);
*poff += count - uio->uio_resid;
VOP_UNLOCK(vp, p);
return (error);


-- 
jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: read(2) on directories

[Chris Cappuccio]

Todd C. Miller [todd.mil...@courtesan.com] wrote:
> On Tue, 12 Jul 2016 12:47:46 -0700, Chris Cappuccio wrote:
> 
> > I've personally always liked being able to cat / read() a directory
> > since it gives you a peek behind the curtain and reflects the
> > reality of how the filesystem is constructed. 
> 
> You haven't been able to do that on OpenBSD for a very long time.
> 

I've been in a deep depression ever since :)

Re: read(2) on directories

[Todd C. Miller]

On Tue, 12 Jul 2016 12:47:46 -0700, Chris Cappuccio wrote:

> I've personally always liked being able to cat / read() a directory
> since it gives you a peek behind the curtain and reflects the
> reality of how the filesystem is constructed. 

You haven't been able to do that on OpenBSD for a very long time.

 - todd

Re: read(2) on directories

[Chris Cappuccio]

Todd C. Miller [todd.mil...@courtesan.com] wrote:
> >From source inspection, Net and Free appear to allow read(2) of
> dirs to succeed.  However, since Linux, Mac OS X and Solaris have
> the EISDIR behavior I think it is probably safe from a portability
> standpoint.
> 
> We're long past the days when opendir(3)/readdir(3) used read(2)...
> 
> HP-UX and AIX still allow reads of directories but no one cares
> about them ;-)
> 

I've personally always liked being able to cat / read() a directory
since it gives you a peek behind the curtain and reflects the
reality of how the filesystem is constructed. 

Re: read(2) on directories

[Todd C. Miller]

>From source inspection, Net and Free appear to allow read(2) of
dirs to succeed.  However, since Linux, Mac OS X and Solaris have
the EISDIR behavior I think it is probably safe from a portability
standpoint.

We're long past the days when opendir(3)/readdir(3) used read(2)...

HP-UX and AIX still allow reads of directories but no one cares
about them ;-)

 - todd

Re: read(2) on directories

[Todd C. Miller]

On Tue, 12 Jul 2016 21:23:51 +0200, Mark Kettenis wrote:

> What do other BSDs (including Mac OS X) do?

Mac OS X returns EISDIR.

 - todd

Re: read(2) on directories

[Tim Newsham]

On Tue, Jul 12, 2016 at 9:19 AM, Todd C. Miller 
wrote:

> Do you know what other systems return EISDIR for read(2) of a
> directory?
>

Linux:
>>> os.open("/", 0)
3
>>> os.read(3, 1024)
Traceback (most recent call last):
  File "", line 1, in 
OSError: [Errno 21] Is a directory


>
>  - todd
>
>


-- 
Tim Newsham | www.thenewsh.com/~newsham | @newshtwit | thenewsh.blogspot.com

Re: read(2) on directories

[Mark Kettenis]

> From: j...@wxcvbn.org (Jeremie Courreges-Anglas)
> Date: Tue, 12 Jul 2016 21:10:37 +0200
> 
> Sending this now to get opinions, but I do not plan any change for 6.0.
> 
> Since I started to use OpenBSD I've always found confusing that
> 
>   cat /directory/
> 
> doesn't error out.  I initially assumed that it was historical behavior,
> but, as hinted by Theo, in rev. 1.1 the behavior was actually to return
> the raw directory entry.
> 
> Both behaviors match POSIX, which allows a third possibility as an XSI
> extension: fail with EISDIR.  I think this is the most useful behavior;
> dumping binary junk is useless, and returning en empty read can hide
> errors.
> 
> I haven't performed extensive testing but if base/xenocara/ports bulk
> builds show errors I can fix them.  The question is, is it worth it?
> 
> Comments / objections?

What do other BSDs (including Mac OS X) do?


> Index: lib/libc/sys/read.2
> ===
> RCS file: /cvs/src/lib/libc/sys/read.2,v
> retrieving revision 1.35
> diff -u -p -r1.35 read.2
> --- lib/libc/sys/read.2   5 Feb 2015 02:33:09 -   1.35
> +++ lib/libc/sys/read.2   9 Jul 2016 17:20:39 -
> @@ -152,13 +152,15 @@ is not a valid file or socket descriptor
>  Part of
>  .Fa buf
>  points outside the process's allocated address space.
> -.It Bq Er EIO
> -An I/O error occurred while reading from the file system.
>  .It Bq Er EINTR
>  A read from a slow device
>  (i.e. one that might block for an arbitrary amount of time)
>  was interrupted by the delivery of a signal
>  before any data arrived.
> +.It Bq Er EIO
> +An I/O error occurred while reading from the file system.
> +.It Bq Er EISDIR
> +The underlying file is a directory.
>  .El
>  .Pp
>  In addition,
> Index: sys/kern/vfs_vnops.c
> ===
> RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
> retrieving revision 1.85
> diff -u -p -r1.85 vfs_vnops.c
> --- sys/kern/vfs_vnops.c  19 Jun 2016 11:54:33 -  1.85
> +++ sys/kern/vfs_vnops.c  9 Jul 2016 17:20:39 -
> @@ -336,11 +336,13 @@ vn_read(struct file *fp, off_t *poff, st
>   if (vp->v_type != VCHR && count > LLONG_MAX - *poff)
>   return (EINVAL);
>  
> + if (vp->v_type == VDIR)
> + return (EISDIR);
> +
>   vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
>   uio->uio_offset = *poff;
> - if (vp->v_type != VDIR)
> - error = VOP_READ(vp, uio,
> - (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0, cred);
> + error = VOP_READ(vp, uio, (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0,
> + cred);
>   *poff += count - uio->uio_resid;
>   VOP_UNLOCK(vp, p);
>   return (error);
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 
> 

Re: read(2) on directories

[Todd C. Miller]

Do you know what other systems return EISDIR for read(2) of a
directory?

 - todd

read(2) on directories

[Jeremie Courreges-Anglas]

Sending this now to get opinions, but I do not plan any change for 6.0.

Since I started to use OpenBSD I've always found confusing that

  cat /directory/

doesn't error out.  I initially assumed that it was historical behavior,
but, as hinted by Theo, in rev. 1.1 the behavior was actually to return
the raw directory entry.

Both behaviors match POSIX, which allows a third possibility as an XSI
extension: fail with EISDIR.  I think this is the most useful behavior;
dumping binary junk is useless, and returning en empty read can hide
errors.

I haven't performed extensive testing but if base/xenocara/ports bulk
builds show errors I can fix them.  The question is, is it worth it?

Comments / objections?


Index: lib/libc/sys/read.2
===
RCS file: /cvs/src/lib/libc/sys/read.2,v
retrieving revision 1.35
diff -u -p -r1.35 read.2
--- lib/libc/sys/read.2 5 Feb 2015 02:33:09 -   1.35
+++ lib/libc/sys/read.2 9 Jul 2016 17:20:39 -
@@ -152,13 +152,15 @@ is not a valid file or socket descriptor
 Part of
 .Fa buf
 points outside the process's allocated address space.
-.It Bq Er EIO
-An I/O error occurred while reading from the file system.
 .It Bq Er EINTR
 A read from a slow device
 (i.e. one that might block for an arbitrary amount of time)
 was interrupted by the delivery of a signal
 before any data arrived.
+.It Bq Er EIO
+An I/O error occurred while reading from the file system.
+.It Bq Er EISDIR
+The underlying file is a directory.
 .El
 .Pp
 In addition,
Index: sys/kern/vfs_vnops.c
===
RCS file: /cvs/src/sys/kern/vfs_vnops.c,v
retrieving revision 1.85
diff -u -p -r1.85 vfs_vnops.c
--- sys/kern/vfs_vnops.c19 Jun 2016 11:54:33 -  1.85
+++ sys/kern/vfs_vnops.c9 Jul 2016 17:20:39 -
@@ -336,11 +336,13 @@ vn_read(struct file *fp, off_t *poff, st
if (vp->v_type != VCHR && count > LLONG_MAX - *poff)
return (EINVAL);
 
+   if (vp->v_type == VDIR)
+   return (EISDIR);
+
vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p);
uio->uio_offset = *poff;
-   if (vp->v_type != VDIR)
-   error = VOP_READ(vp, uio,
-   (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0, cred);
+   error = VOP_READ(vp, uio, (fp->f_flag & FNONBLOCK) ? IO_NDELAY : 0,
+   cred);
*poff += count - uio->uio_resid;
VOP_UNLOCK(vp, p);
return (error);


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE