Allow fuchsia to open non-regular files.

Allow fuchsia to open non-regular files.

BUG=

[anwilson, 2017-04-26 17:01:59]

Description was changed from

==========
Allow fuchsia to open non-regular files.

BUG=
==========

to

==========
Allow fuchsia to open non-regular files.

BUG=
==========

[anwilson, 2017-04-26 17:01:59]

anwilson@google.com changed reviewers:
+ abarth@google.com

[anwilson, 2017-04-26 17:29:51]

anwilson@google.com changed reviewers:
+ zra@google.com

[zra, 2017-04-26 17:34:13]

https://codereview.chromium.org/2844493004/diff/1/runtime/bin/file_fuchsia.cc
File runtime/bin/file_fuchsia.cc (left):

https://codereview.chromium.org/2844493004/diff/1/runtime/bin/file_fuchsia.cc...
runtime/bin/file_fuchsia.cc:198: if (NO_RETRY_EXPECTED(stat(name, &st)) == 0) {
Could you explain why this is needed?

This change removes the failure if the path is for a directory.

It looks like there was a change on the Linux side that wasn't ported here.
Maybe it would help. I.e.:

if (TEMP_FAILURE_RETRY(stat64(name, &st)) == 0) {
    // Only accept regular files, character devices, and pipes.
    if (!S_ISREG(st.st_mode) && !S_ISCHR(st.st_mode) && !S_ISFIFO(st.st_mode)) {
      errno = (S_ISDIR(st.st_mode)) ? EISDIR : ENOENT;
      return NULL;
    }
  }

[abarth, 2017-04-26 17:42:14]

On 2017/04/26 at 17:34:13, zra wrote:
> https://codereview.chromium.org/2844493004/diff/1/runtime/bin/file_fuchsia.cc
> File runtime/bin/file_fuchsia.cc (left):
> 
>
https://codereview.chromium.org/2844493004/diff/1/runtime/bin/file_fuchsia.cc...
> runtime/bin/file_fuchsia.cc:198: if (NO_RETRY_EXPECTED(stat(name, &st)) == 0)
{
> Could you explain why this is needed?

We have lots of files on Fuchsia that have exotic types that are still useful to
interact with via the Dart File interface.  Specifically, Andrew wants to write
data to /dev/misc/dmctl.

> This change removes the failure if the path is for a directory.

Maybe we should blacklist directory.  It seems unlikely that directories will be
useful to interact with via File.

> It looks like there was a change on the Linux side that wasn't ported here.
Maybe it would help. I.e.:
> 
> if (TEMP_FAILURE_RETRY(stat64(name, &st)) == 0) {
>     // Only accept regular files, character devices, and pipes.
>     if (!S_ISREG(st.st_mode) && !S_ISCHR(st.st_mode) && !S_ISFIFO(st.st_mode))
{
>       errno = (S_ISDIR(st.st_mode)) ? EISDIR : ENOENT;
>       return NULL;
>     }
>   }

I don't think a whitelist is going to work for us.  There's lots of fun stuff in
the file system to interact with that we can't even really explain what they is
via stat64.

[abarth, 2017-04-26 17:42:36]

On 2017/04/26 at 17:42:14, abarth wrote:
> On 2017/04/26 at 17:34:13, zra wrote:
> >
https://codereview.chromium.org/2844493004/diff/1/runtime/bin/file_fuchsia.cc
> > File runtime/bin/file_fuchsia.cc (left):
> > 
> >
https://codereview.chromium.org/2844493004/diff/1/runtime/bin/file_fuchsia.cc...
> > runtime/bin/file_fuchsia.cc:198: if (NO_RETRY_EXPECTED(stat(name, &st)) ==
0) {
> > Could you explain why this is needed?
> 
> We have lots of files on Fuchsia that have exotic types that are still useful
to interact with via the Dart File interface.  Specifically, Andrew wants to
write data to /dev/misc/dmctl.
> 
> > This change removes the failure if the path is for a directory.
> 
> Maybe we should blacklist directory.  It seems unlikely that directories will
be useful to interact with via File.
> 
> > It looks like there was a change on the Linux side that wasn't ported here.
Maybe it would help. I.e.:
> > 
> > if (TEMP_FAILURE_RETRY(stat64(name, &st)) == 0) {
> >     // Only accept regular files, character devices, and pipes.
> >     if (!S_ISREG(st.st_mode) && !S_ISCHR(st.st_mode) &&
!S_ISFIFO(st.st_mode)) {
> >       errno = (S_ISDIR(st.st_mode)) ? EISDIR : ENOENT;
> >       return NULL;
> >     }
> >   }
> 
> I don't think a whitelist is going to work for us.  There's lots of fun stuff
in the file system to interact with that we can't even really explain what they
is via stat64.

*they are*

[zra, 2017-04-26 17:55:30]

On 2017/04/26 17:42:36, abarth wrote:
> On 2017/04/26 at 17:42:14, abarth wrote:
> > On 2017/04/26 at 17:34:13, zra wrote:
> > >
> https://codereview.chromium.org/2844493004/diff/1/runtime/bin/file_fuchsia.cc
> > > File runtime/bin/file_fuchsia.cc (left):
> > > 
> > >
>
https://codereview.chromium.org/2844493004/diff/1/runtime/bin/file_fuchsia.cc...
> > > runtime/bin/file_fuchsia.cc:198: if (NO_RETRY_EXPECTED(stat(name, &st)) ==
> 0) {
> > > Could you explain why this is needed?
> > 
> > We have lots of files on Fuchsia that have exotic types that are still
useful
> to interact with via the Dart File interface.  Specifically, Andrew wants to
> write data to /dev/misc/dmctl.
> > 
> > > This change removes the failure if the path is for a directory.
> > 
> > Maybe we should blacklist directory.  It seems unlikely that directories
will
> be useful to interact with via File.
> > 
> > > It looks like there was a change on the Linux side that wasn't ported
here.
> Maybe it would help. I.e.:
> > > 
> > > if (TEMP_FAILURE_RETRY(stat64(name, &st)) == 0) {
> > >     // Only accept regular files, character devices, and pipes.
> > >     if (!S_ISREG(st.st_mode) && !S_ISCHR(st.st_mode) &&
> !S_ISFIFO(st.st_mode)) {
> > >       errno = (S_ISDIR(st.st_mode)) ? EISDIR : ENOENT;
> > >       return NULL;
> > >     }
> > >   }
> > 
> > I don't think a whitelist is going to work for us.  There's lots of fun
stuff
> in the file system to interact with that we can't even really explain what
they
> is via stat64.
> 
> *they are*

Okay. Updating this to instead blacklist directories sounds good to me.

[zra, 2017-05-02 19:35:57]

Is this still needed? I can take over this CL if that would be easier.

[anwilson, 2017-05-02 20:46:23]

On 2017/05/02 19:35:57, zra wrote:
> Is this still needed? I can take over this CL if that would be easier.

Yeah if you could take it over that would be great.  Thanks!

[anwilson, 2017-05-02 20:46:24]

On 2017/05/02 19:35:57, zra wrote:
> Is this still needed? I can take over this CL if that would be easier.

Yeah if you could take it over that would be great.  Thanks!

[zra, 2017-05-02 21:28:27]

On 2017/05/02 20:46:23, anwilson wrote:
> On 2017/05/02 19:35:57, zra wrote:
> > Is this still needed? I can take over this CL if that would be easier.
> 
> Yeah if you could take it over that would be great.  Thanks!

Picked up over here: https://codereview.chromium.org/2856913004/