bug#45198: 28.0.50; Sandbox mode

[Philipp Stephani]

Am Di., 29. Dez. 2020 um 18:09 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Tue, 29 Dec 2020 17:05:33 +0100
> > Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Bastien <bzg@gnu.org>, 
> > 45198@debbugs.gnu.org,
> >       João Távora <joaotavora@gmail.com>
> >
> > Am Di., 29. Dez. 2020 um 16:44 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> > >
> > > > It would be great if someone could test whether the Windows build
> > > > still works after that, thanks!
> > >
> > > I only skimmed the changes, but I'm already quite sure they will break
> > > the Windows build.
> >
> > Why? Everything should be behind ifdefs/conditional compilation,
> > otherwise compilation on macOS would also break.
>
> Because some/many Gnulib replacements are incompatible with how the
> w32 port of Emacs works.  In particular, functions which operate on
> file names don't support UTF-8 encoded file names; Gnulib's 'stat' and
> 'fstat' don't support security-related features from which we glean
> owner and group of files; network-related functions need special
> handling in Emacs to support subprocess functionality; etc.

That's a fair point, but does the mere presence of these replacements
really break the build? Could we somehow make them not break the
build?

>
> > >   . Gnulib modules pulled to support seccomp should be disabled in the
> > >     MS-Windows build, by suitable changes to nt/mingw-cfg.site and/or
> > >     nt/gnulib-cfg.mk; and
> >
> > This shouldn't be necessary, as Gnulib is compatible with Windows (I
> > guess, since we use it elsewhere)
>
> Not when Emacs is concerned, see above.  We use only small parts of
> Gnulib on Windows, for those reasons.  And we don't use any Gnulib
> functions that accept file names.
>
> > (That's also what gnulib-cfg.mk says: "In general, do NOT omit modules
> > that don't need to be omitted, to minimize the differences from
> > upstream gnulib.mk and thus make the maintenance easier.")
>
> I know; I wrote that.  However, this talks about code that will be
> actually _used_ in the Windows build, whereas here we are talking
> about "ballast": the related code will not be used at all.  So it
> makes sense not to make our job harder by adding unnecessary code,
> which then will need to be audited for compatibility.  Your Gnulib
> import adds quite a lot of modules, which makes the audit a large job.

Independent of this discussion, are there ways to reduce the size of
the auditing job? I'd hope that eventually the only thing needed would
be to run the test suite (which ideally would happen automatically on
Gitlab/Emba).
For developers not on Windows, it's unfortunately difficult to predict
whether a specific change will break the Windows build or not, and the
large difference between the Windows and other builds doesn't make
this easier.

>
> > >   . header files used to support seccomp code should be #ifdef'ed by
> > >     HAVE_LINUX_SECCOMP_H or similar, and likewise with any code needed
> > >     for seccomp and unnecessary otherwise.
> >
> > That should already be the case.
>
> It isn't, at least not in general.  Just one example:
>
>   diff --git a/src/emacs.c b/src/emacs.c
>   index 2a32083..a044535 100644
>   --- a/src/emacs.c
>   +++ b/src/emacs.c
>   @@ -33,6 +33,8 @@ along with GNU Emacs.  If not, see 
> <https://www.gnu.org/licenses/>.  */
>    #include "lisp.h"
>    #include "sysstdio.h"
>
>   +#include <read-file.h>  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>
> This header is included unconditionally, although it shouldn't be
> needed on Windows.

Sure, it's not needed, but does it actually break the build? I'd
rather limit the number of preprocessor statements as much as possible
(and reasonable).

>
> > Turning the question around: is building the branch on Windows
> > actually broken? If so, what are the error messages?
>
> I didn't have time to build it, and probably won't have enough time in
> the following days, sorry.  I have the co-maintainer job to do, and
> there's a pretest of Emacs 27.2 under way on top of that.  So I cannot
> show the error messages.  Others are encouraged to try the branch and
> report the actual problems; I just wanted to give a heads-up, in the
> hope that it will help adapting and merging the branch sooner.

Thanks, no pressure, this clearly isn't urgent.

>
> > > Btw, I wonder why you needed to import the read-file module from
> > > Gnulib -- does it provide any features that we couldn't implement on
> > > our own?  I'm asking because that module caused you to pull in quite a
> > > few dependency modules from Gnulib, and I'm asking myself whether that
> > > is really justified.
> >
> > We could implement it ourselves, if we wanted, and in an earlier
> > version of the code I did that. But it's easier and less error-prone
> > to reuse an existing library.
>
> I question the wisdom of that, FWIW.  Every unnecessary dependency on
> Gnulib is IMO an addition to the maintenance burden.  It also makes
> the job of adapting the Windows build harder, because for example
> Gnulib's fopen cannot be used in Emacs on Windows, whereas we have
> emacs_fopen for the same purpose, which doesn't have that problem.

Yeah, there are pros and cons for either approach. Typically, reusing
code is the way to go, since code is a liability, and the less we have
to maintain, the better. But I do see your point that using Gnulib is
too much of a burden for Windows right now (though I still hope that
we can eventually improve Gnulib enough that we can use it directly on
Windows). I've now created a new branch scratch/seccomp-no-gnulib-2
that uses fopen directly, without Gnulib dependencies. (I considered
using emacs_fopen, but since that allows users to quit and therefore
in theory can run Lisp code, I'm not sure we can use it here. Also,
since this only works on GNU/Linux, we don't have to deal with Windows
filenames.)