Skip to content

Loading…

v0.5.9 fails to return EISDIR error on fs.read() of a directory #1883

Closed
tshinnic opened this Issue · 6 comments

3 participants

@tshinnic

Noticed this while using fs.createReadStream() and 'bad' filename that was actually a subdirectory.
(test 0.5.x with bad data too!)
This is on Fedora 15 Linux.

Tracked down to difference in fs.read() error returns:

        [Tom@TLSF15A temp]$ node -v
    >>  v0.4.12
        > [Tom@TLSF15A temp]$ node
        > var fs = require('fs')
        > var z = fs.openSync('subdir','r')
        > var s = fs.readSync(z,16,null,'utf8')
    >>  Error: EISDIR, Is a directory
            at Object.readSync (fs.js:264:19)


        [Tom@TLSF15A temp]$ ../node5 -v
    >>  v0.5.9
        [Tom@TLSF15A temp]$ ../node5
        > var fs = require('fs')
        > var z = fs.openSync('subdir','r')
        > var s = fs.readSync(z,16,null,'utf8')
    >>  Error: UNKNOWN, Unknown error
            at Object.readSync (fs.js:265:19)

The cited line in fs.js is

  var r = binding.read(fd, buffer, offset, length, position);

Now I'll go check against latest master...

@tshinnic

Checked out latest node and problem still exists:

  [Tom@TLSF15A temp]$ ../../node5p -v
  v0.5.10-pre
  [Tom@TLSF15A temp]$ ../../node5p
  >       var fs = require('fs')
  >       var util = require('util')
  >       var z = fs.openSync('subdir','r')
  >       var s = fs.readSync(z,16,null,'utf8')
  Error: UNKNOWN, Unknown error
      at Object.readSync (fs.js:265:19)
@tshinnic

Ahh, libuv doesn't yet know about some 'standard' errno code values?
Everybody uses uv__set_sys_error() to remember errno values:

  deps/uv/src/uv-common.c:106:void uv__set_sys_error(uv_loop_t* loop, int sys_error) {
            void uv__set_sys_error(uv_loop_t* loop, int sys_error) {
              loop->last_err.code = uv_translate_sys_error(sys_error);
              loop->last_err.sys_errno_ = sys_error;
            }

Routine uv_err_code uv_translate_sys_error() is supposed to translate from system error code values to libuv system-independent code values:

  ../deps/uv/src/unix/error.c
       86 uv_err_code uv_translate_sys_error(int sys_errno) {
       87   switch (sys_errno) {
       88     case 0: return UV_OK;
       89     case ENOSYS: return UV_ENOSYS;
       90     case ENOENT: return UV_ENOENT;
            :        :        :        :        :
      104     case ENOTCONN: return UV_ENOTCONN;
      105     case EEXIST: return UV_EEXIST;
      106     default: return UV_UNKNOWN;
      107   }

If an 'unknown' errno value is seen, the result defaults to UV_UNKNOWN, resulting eventually in the semi-bogus display:

      Error: UNKNOWN, Unknown error

With printf()'s inline in uv_fs_read() for debugging we see the defaulted "I don't know that one" value:

  uv_fs_read() sees result -1, errno 21
               last_err.sys_errno    21
               last_err.code         -1

As an experiment, add a bogus translation from EISDIR to a valid libuv error code in deps/uv/src/unix/error.c

  --- error.c.original    2011-10-13 17:01:33.700816284 -0500
  +++ error.c     2011-10-13 18:24:33.876709373 -0500
  @@ -103,6 +103,7 @@
       case ENOTDIR: return UV_ENOTDIR;
       case ENOTCONN: return UV_ENOTCONN;
       case EEXIST: return UV_EEXIST;
  +    case EISDIR: return UV_ECONNREFUSED;
       default: return UV_UNKNOWN;
     }

and now testing displays

  [Tom@TLSF15A temp]$ /home/Tom/study/javascript/node.js/node_repo_20111013_1701/node test01.js
    uv_fs_read() sees result -1, errno 21
                 last_err.sys_errno    21
                 last_err.code         11

  Error: ECONNREFUSED, Connection refused
      at Object.readSync (fs.js:265:19)

.
How many more valid and useful system error errno values still need to be applied to v0.5.10pre in order to make libuv platform "ready for production"? (Especially for *nix platforms which have been directly reflecting system codes till now)

How difficult/tedious/gross is adding to the set of UV_nnnnnn error code definitions?

@bnoordhuis
Node.js Foundation member

How many more valid and useful system error errno values still need to be applied to v0.5.10pre in order to make libuv > platform "ready for production"? (Especially for *nix platforms which have been directly reflecting system codes till now)

How difficult/tedious/gross is adding to the set of UV_nnnnnn error code definitions?

Pretty tedious. It's been done a haphazard way until now. I'll put something more solid in place.

@tshinnic

I'm groping for a way to enumerate/quantify/qualify which of the known system code errno values are 'needed' - should be included within the UV_nnnnn set.

One idea would be to grep for known codes as used in existing user code. As one (obvious) example I chose npm:

    lib/completion.js:            EPIPE
    lib/utils/error-handler.js:   EACCES
    lib/utils/error-handler.js:   ECONNREFUSED
    lib/utils/error-handler.js:   EEXIST
    lib/utils/error-handler.js:   EPERM
    lib/utils/exec.js:            EPERM
    lib/utils/lifecycle.js:       EPERM
    lib/utils/mkdir-p.js:         EEXIST
    lib/utils/output.js:          EPIPE

Checking in ./deps/uv/include.uv.h for whether these are defined :

  . EACCES
  . ECONNREFUSED
  . EEXIST
    EPERM
  . EPIPE

finds that EPERM is not defined in ./deps/uv/include.uv.h

Another idea would be to check source within node. For example, what codes might be returned from deps/uv/src/unix/eio/eio.c ?

    #ifndef ECANCELED
    # define ECANCELED EDOM
    #ifndef ELOOP
    # define ELOOP EDOM
    #if !defined(ENOTSOCK) && defined(WSAENOTSOCK)
    # define ENOTSOCK WSAENOTSOCK

  . EACCES
  . EAFNOSUPPORT
  . EAGAIN
  . EBADF
    ECANCELED
  . EINTR
  . EINVAL
    EIO
    ELOOP
    ENAMETOOLONG
  . ENOENT
  . ENOMEM
  . ENOSYS
  . ENOTDIR
  . ENOTSOCK
    EOPNOTSUPP
  . EPROTOTYPE
  . ETIMEDOUT

This finds that ECANCELLED, EIO, ELOOP, ENAMETOOLONG, and EOPNOTSUPP could likely be returned from eio, and yet these are not known within the libuv codes.

What other ideas are there for 'polling' which codes are "important enough", besides laboriously going through packages and node sources?

.
Please note that it would be wonderful if any additional work on organizing uv_code <--> code translations and code <--> name translations could integrate with the concern behind #1689 - how to bring combined error handling to the JS level.

(see https://github.com/joyent/node/blob/master/lib/net.js#L577 (and the other 3 instances of same))

@trevnorris

It looks like this bug is more open ended, as it suggests adding "useful system errors". @bnoordhuis looks like many more have been added since this bug was last updated. Would you say they are sufficient for "production ready"?

@bnoordhuis
Node.js Foundation member

looks like many more have been added since this bug was last updated. Would you say they are sufficient for "production ready"?

Yes. Closing, thanks.

@bnoordhuis bnoordhuis closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.