Re: [PATCH] Implement open-process and related functions on MinGW

[Mark H Weaver]

Hi Eli,

Eli Zaretskii <address@hidden> writes:

>> From: Mark H Weaver <address@hidden>
>> Cc: address@hidden (Ludovic Courtès),  address@hidden
>> Date: Sat, 22 Feb 2014 10:54:16 -0500
>> 
>> > diff --git a/libguile/posix.c b/libguile/posix.c
>> > index 0443f95..69652a3 100644
>> > --- a/libguile/posix.c
>> > +++ b/libguile/posix.c
>> > @@ -85,6 +85,27 @@
>> >  #if HAVE_SYS_WAIT_H
>> >  # include <sys/wait.h>
>> >  #endif
>> > +#ifdef __MINGW32__
>> > +# define WEXITSTATUS(stat_val) ((stat_val) & 255)
>> > +/* Windows reports exit status from fatal exceptions as 0xC0000nnn
>> > +   codes, see winbase.h.  We usurp codes above 0xC0000200 for SIGxxx
>> > +   signals.  */
>> > +# define WIFEXITED(stat_val)   (((stat_val) & 0xC0000000) == 0)
>> > +# define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) != 0)
>> > +# define WTERMSIG(stat_val)    ((stat_val > 0xC0000200) ? stat_val - 
>> > 0xC0000200 : stat_val)
>> > +# define WIFSTOPPED(stat_val)  (0)
>> > +# define WSTOPSIG(stat_var)    (0)
>> 
>> Definitions for these on MinGW are available in Gnulib's 'sys_wait'
>> module.  I'll import it soon.
>
> Please don't: that gnulib module is dead wrong for Windows.  If we use
> it, we will be unable to report even the simplest error exits, let
> alone programs that crashed due to a fatal signal.

Hmm.  This is rather uncomfortable.  Did you discuss these problems with
the Gnulib developers?  If so, can you provide a link to the relevant
threads?

Your comment above "We usurp codes above 0xC0000200 for SIGxxx signals"
makes me wonder: How widely used is this convention?  What other
software packages use it?

>> > +# include <process.h>
>> > +# define waitpid(p,s,o)        _cwait(s, p, WAIT_CHILD)
>> > +# define HAVE_WAITPID 1
>> 
>> Gnulib has a 'waitpid' module that implements it on MinGW.  Can we just
>> use that, and then assume it is there instead of setting HAVE_WAITPID?
>
> The gnulib waitpid module does exactly what I did above.  If you are
> more comfortable with including it than with a single trivial macro,
> then go ahead.

In general, I would prefer to use Gnulib, except in cases where their
MinGW wrappers are not good enough to get the job done properly.

My reasons for this preference are (1) to keep the Guile code cleaner,
and (2) if some deficiency is found in the wrapper, or if Windows
introduces an improved API some day, the changes have to be made in only
one place: Gnulib.

> But in general, due to the known problems with getting
> Windows-specific patches to gnulib approved and admitted, I'd advise
> to stay away of gnulib as much as possible, when Windows is concerned.

Can you provide some links to mailing list threads where you've tried to
get things fixed in Gnulib and encountered resistance?

> E.g., what if tomorrow we would need to make the waitpid emulation
> more sophisticated?

I suppose I would prefer to get the problem fixed in Gnulib.  If there
is truly a problem with the way Gnulib is being maintained w.r.t. to
Windows, perhaps I will make an effort to get that larger problem fixed.

Having said all of this, I'm not necessarily opposed to using Windows
APIs directly when there is a clear benefit to doing so.

For example, if it is true that we can avoid all the nasty problems with
filename encoding by using the native Windows APIs that use UTF-16 for
filenames, then I'd be in favor of doing that.

I'd also be in favor of using native Windows APIs to spawn new processes
if that's the only way to do it properly.  (See below)

>> > +# define getuid()              (500) /* Local Administrator */
>> > +# define getgid()              (513) /* None */
>> > +# define setuid(u)             (0)
>> > +# define setgid(g)             (0)
>> 
>> Hmm.  I'm not sure about these.  If we can't do better than this, I
>> think we should arrange to not bind these functions in MinGW, and not
>> call them in our code.  What do you think?
>
> Which functions are you talking about, specifically?

The ones that use the macros you've defined above,
e.g. {get,set}{uid,gid,euid,egid}.

> In general, where the Windows equivalent is to do nothing, my advice
> would be to have a no-op implementation, rather than an unbound
> function.  The former approach makes it much easier to write portable
> Guile scripts, instead of spreading boundness checks all over the
> place.

I think this is a very bad idea.  First of all, it's not very common for
Guile programs to query or set the uid/gid, so I don't see much benefit.

On the other hand, if a program _does_ try to do one of those things, it
might be important that the job be done right.  For example, a program
might be trying to reduce its privileges.  Doing nothing while silently
pretending that the operation succeeded is a good way to cause security
flaws to go unnoticed, or to make a programmer waste hours of time
trying to debug a problem that he should have been notified about
immediately with an error message.

> FWIW, Emacs takes the former approach with very good results.

Emacs is primarily an interactive editor, and therefore it is more
acceptable for it to try to make guesses and paper over problems for the
sake of convenience.  Guile is a general-purpose programming language,
and we must consider the needs of a larger set of users, for example the
authors of server software where security is important.

>> > +# define pipe(f)               _pipe(f, 0, O_NOINHERIT)
>> 
>> Gnulib has a 'pipe' module that implements it on MinGW.
>
> Same response as for waitpid.

Same reply as for waitpid.

>> Can you use the GNU coding conventions for placement of brackets?
>
> Sorry, I don't follow: which part is not according to the GNU coding
> conventions?

Nevermind, my mistake.  Your use of tabs in the indentation combined
with the prefix characters in the email caused some (but not all) lines
to move to the right.  I've long been using (setq indent-tabs-mode nil)
globally, for that reason.

[...]

>> Thanks for working on this, but in a multithreaded program, it's no good
>> to change the file descriptors in the main program temporarily before
>> spawning, and then restore them afterwards.  We'll have to find another
>> way of doing this.
>
> Threads don't work in the MinGW build anyway, and no one was able to
> help me understand why (see the discussions last August).  As long as
> this limitation exists, this is a non-issue, and certainly better than
> not having this functionality available at all.

As I wrote elsewhere, madsy on #guile reported having successfully built
a recent Guile snapshot with both threads and posix enabled.

I know that you asked for more specifics about this, but I don't know
the answers.  I asked madsy to reply to you directly when he has some
free time.  However, I remember that he cross-built from Ubuntu, and
iirc he built libgc (and several other dependencies) from source code.
     
> The only other way I know of is to bypass Posix-like functions like
> 'open', 'close', and 'dup', and go to the level of Win32 API, where a
> function that creates a child subprocess can accept handles for the
> child's standard I/O.

This sounds to me like the right approach in this case.

> If you will be comfortable with a lot more Widows specific stuff, I
> can work on that.

Please do!

    Thanks,
      Mark