Re: svn commit: r1702305 - /subversion/trunk/subversion/libsvn_subr/stream.c

Stefan Fuhrmann <stefan2_at_apache.org> writes:

+ * Special files like STDIN will have no flags set at all. In that case,
+ * we can't filter and must allow any operation - which may then fail at
+ * the APR level further down the stack.
+ */
+ apr_uint32_t flags = apr_file_flags_get(file);
+ svn_boolean_t supports_read = (flags == 0) || (flags & APR_READ);
+ svn_boolean_t supports_write = (flags == 0) || (flags & APR_WRITE);

Files corresponding to the standard I/O streams actually have the appropriate
APR_READ / APR_WRITE flags set starting from APR 1.3 — see, for example,
apr_file_open_flags_stderr() in [1]. Hence, the (flags == 0) check is going
to return false for them.

> Note that this check is not perfect for arbitrary APR file handles
> (may enable more functions than the handle actually supports) but
> works correctly for our STD* streams and the files opened through
> our svn_io_* API.

The actual problem, to my mind, is that relying on flags to determine if the
file allows reading or writing, is fragile. There are examples of apr_file_t's
that don't have the corresponding flags, but still allow reading and writing,
and svn_stream_from_aprfile2() is going to break for them.

One example would be apr_file_pipe_create() on Unix that sets APR_INHERIT
flag on the created pipe [2]. Another example is creating a pipe on Windows
that currently initializes flags to zero [3].

[1] https://svn.apache.org/repos/asf/apr/apr/branches/1.3.x/file_io/unix/open.c
[2] https://svn.apache.org/repos/asf/apr/apr/trunk/file_io/unix/pipe.c
[3] https://svn.apache.org/repos/asf/apr/apr/trunk/file_io/win32/pipe.c

Regards,
Evgeny Kotkov
Received on 2015-09-10 23:50:38 CEST