On Thu, 27 Oct 2005, Philip Martin wrote:
> lundblad@tigris.org writes:
>
> > --- trunk/subversion/libsvn_subr/io.c (original)
> > +++ trunk/subversion/libsvn_subr/io.c Wed Oct 26 14:14:12 2005
> > @@ -1733,10 +1733,14 @@
> > const char *desc)
> > {
> > char errbuf[256];
> > + apr_file_t *stderr_handle;
> > +
> > + apr_file_open_stderr (&stderr_handle, pool);
> >
> > /* What we get from APR is in native encoding. */
> > - fprintf (stderr, "%s: %s", desc, apr_strerror (status, errbuf,
> > - sizeof (errbuf)));
> > + apr_file_printf (stderr_handle, "%s: %s",
> > + desc, apr_strerror (status, errbuf,
> > + sizeof (errbuf)));
>
> The lack of error handling prompted me to look at the implementation
> of apr_file_open_stderr, it's basically just a wrapper around a
> hard-coded file descriptor 2. A library cannot use that as there is
> no guarantee that 2 will be open, or that is stderr if it is open.
> If the application has closed stderr you may be writing to a random
> file.
>
Funny. We start to go in circles here. As I've pointed out earlier, since
the child process already will write to stderr in some cases (dynamic
linker), so if stderr is closed and another file is open with descriptor
2, then you already have that risk.
You could argue that the caller might really "know" that the called
program will never write to stderr, but I consider that a very special
case that won't happen in practice. I mean that assuming a program will
not under any circumstances write to stderr isn't very portable.
(Regarding the lack of error handling, that's sloppy and actually could be
a problem on Windows. I looked at the libsvn_ra_svn/client.c code and
assumed it was correct, and that were nothing to do in this case, so
writing to the file was not a problem. As it turns out, on error, the file
descriptor is invalid. I'll fix both places separately. Sigh!)
> I think svn_io_start_cmd needs to pass errfile (possibly null)
via
> pool userdata, and handle_child_process_error needs to retrieve it and
> then write to it if it's not null. If errfile is null I think the
> error message simply has to be dropped.
This has also been suggested in some form. The problem is that one pool
could well be used to start two commands in sequence. How do you name the
userdata then?
Thansk,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Oct 27 08:39:42 2005