bug#25895: Remove update-game-score

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

Eli Zaretskii wrote:

> Please add comments to the affected Makefile.in files to explain the
> conditions related to user/group.

Done in the attached revised patch, which also fixes the bug Ulrich Müller noted.

> I also don't understand why you unconditionally removed this program
> from the Windows builds and installations: the problem with setgid
> doesn't exist on Windows, so nothing should prevent Windows
> installations from having this program, right?

It's more the other way round. On platforms without setuid/setgid, Emacs can use 
its already-existing code to update the score file itself. The auxiliary program 
is needed only on platforms that have setuid/setgid, to avoid the security 
problems that would ensue if we made Emacs itself setuid/setgid.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Install-update-game-score-only-on-request.patch --]
[-- Type: text/x-diff; name="0001-Install-update-game-score-only-on-request.patch", Size: 15367 bytes --]

From 659d605d6456779be0c6703eceda508b76e751f0 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 12 Mar 2017 00:17:42 -0800
Subject: [PATCH] Install update-game-score only on request

Most distributions do not install update-game-score properly
due to setuid/setgid complications, so install it only when
the installer specifies a user or group (Bug#25895).
* .gitattributes: Remove lib-src/update-game-score.exe.manifest.
* Makefile.in (gameuser, gamegroup, use_gamedir, PATH_GAME):
New vars.
(epaths-force): Use PATH_GAME.
(uninstall): Remove snake-scores and tetris-scores only if shared.
* configure.ac: Default --with-gameuser to 'no'.
(UPDATE_MANIFEST): Remove.
* etc/NEWS: Mention this.
* lib-src/Makefile.in (UPDATE_MANIFEST): Remove.
(use_gamedir): New macro.
(UTILITIES): Remove update-game-score unless use_gamedir.
(SCRIPTS): Remove $(UPDATE_MANIFEST).
($(DESTDIR)${archlibdir}): Install game directory program and data
only if use_gamedir.
* lib-src/update-game-score.exe.manifest: Remove, as
update-game-score is no longer installed on MS-Windows.
* lisp/play/gamegrid.el (gamegrid-add-score-with-update-game-score):
Use auxiliary program only if setuid or setgid.
* make-dist: Do not distribute update-game-score.exe.manifest.
* src/callproc.c (init_callproc):
Set Vshared_game_score_directory based on PATH_GAME, not DOS_NT.
(syms_of_callproc): Remove unnecessary initialization of
Vshared_game_score_directory.
---
 .gitattributes                         |  1 -
 Makefile.in                            | 13 ++++++++--
 configure.ac                           | 12 +++------
 etc/NEWS                               |  5 ++++
 lib-src/Makefile.in                    | 45 ++++++++++++++--------------------
 lib-src/update-game-score.exe.manifest | 10 --------
 lisp/play/gamegrid.el                  | 29 ++++++++--------------
 make-dist                              |  1 -
 src/callproc.c                         | 20 ++++++---------
 9 files changed, 56 insertions(+), 80 deletions(-)
 delete mode 100644 lib-src/update-game-score.exe.manifest

diff --git a/.gitattributes b/.gitattributes
index 22cea3d..59cc2ed 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -24,7 +24,6 @@ admin/charsets/mapfiles/PTCP154 whitespace=cr-at-eol
 leim/MISC-DIC/cangjie-table.b5 whitespace=cr-at-eol
 leim/MISC-DIC/cangjie-table.cns whitespace=cr-at-eol
 leim/MISC-DIC/pinyin.map whitespace=cr-at-eol
-lib-src/update-game-score.exe.manifest whitespace=cr-at-eol
 nt/nmake.defs whitespace=cr-at-eol
 test/etags/c-src/dostorture.c whitespace=cr-at-eol
 test/etags/cp-src/c.C whitespace=cr-at-eol
diff --git a/Makefile.in b/Makefile.in
index 82fb91f..bf673f4 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -266,8 +266,12 @@ archlibdir=
 # Where to put the etc/DOC file.
 etcdocdir=@etcdocdir@
 
-# Where to install Emacs game score files.
+# Where to install game score files, if gameuser or gamegroup is nonempty.
 gamedir=@gamedir@
+gameuser=@gameuser@
+gamegroup=@gamegroup@
+# Nonempty if and only if a shared gamedir is used.
+use_gamedir=$(gameuser)$(gamegroup)
 
 # ==================== Utility Programs for the Build ====================
 
@@ -334,6 +338,9 @@ etc-emacsver:
 	  ${srcdir}/build-aux/move-if-change emacsver.tex.$$$$ \
 	  ${srcdir}/etc/refcards/emacsver.tex
 
+# The shared gamedir name as a C string literal, or a null ptr if not in use.
+PATH_GAME = $(if $(use_gamedir),"$(gamedir)",((char const *) 0))
+
 # Generate epaths.h from epaths.in.  This target is invoked by 'configure'.
 # See comments in configure.ac for why it is done this way, as opposed
 # to just letting configure generate epaths.h from epaths.in in a
@@ -359,7 +366,7 @@ epaths-force:
 	  -e 's;\(#.*PATH_DATA\).*$$;\1 "${etcdir}";'			\
 	  -e 's;\(#.*PATH_BITMAPS\).*$$;\1 "${bitmapdir}";'		\
 	  -e 's;\(#.*PATH_X_DEFAULTS\).*$$;\1 "${x_default_search_path}";' \
-	  -e 's;\(#.*PATH_GAME\).*$$;\1 "${gamedir}";' \
+	  -e 's;\(#.*PATH_GAME\).*$$;\1 $(PATH_GAME);'			\
 	  -e 's;\(#.*PATH_DOC\).*$$;\1 "${etcdocdir}";') &&		\
 	${srcdir}/build-aux/move-if-change epaths.h.$$$$ src/epaths.h
 
@@ -811,10 +818,12 @@ uninstall:
 	-rm -f "$(DESTDIR)${desktopdir}/${EMACS_NAME}.desktop"
 	-rm -f "$(DESTDIR)${appdatadir}/${EMACS_NAME}.appdata.xml"
 	-rm -f "$(DESTDIR)$(systemdunitdir)/${EMACS_NAME}.service"
+  ifneq (,$(use_gamedir))
 	for file in snake-scores tetris-scores; do \
 	  file="$(DESTDIR)${gamedir}/$${file}"; \
 	  [ -s "$${file}" ] || rm -f "$$file"; \
 	done
+  endif
 
 ### Windows-specific uninstall target for removing programs produced
 ### in nt/, and its Posix do-nothing shadow.
diff --git a/configure.ac b/configure.ac
index ba944e6c..4d9ba96 100644
--- a/configure.ac
+++ b/configure.ac
@@ -402,12 +402,9 @@ AC_DEFUN
 		An argument prefixed by ':' specifies a group instead.])])
 gameuser=
 gamegroup=
-# We don't test if we can actually chown/chgrp here, because configure
-# may run without root privileges.  lib-src/Makefile.in will handle
-# any errors due to missing user/group gracefully.
 case ${with_gameuser} in
-  no) ;;
-  "" | yes) gamegroup=games ;;
+  '' | no) ;;
+  yes) gamegroup=games ;;
   :*) gamegroup=${with_gameuser#:} ;;
   *) gameuser=${with_gameuser} ;;
 esac
@@ -1966,7 +1963,6 @@ AC_DEFUN
 CLIENTW=
 W32_RES_LINK=
 EMACS_MANIFEST=
-UPDATE_MANIFEST=
 if test "${with_w32}" != no; then
   case "${opsys}" in
     cygwin)
@@ -2034,7 +2030,6 @@ AC_DEFUN
     # the rc file), not a linker script.
     W32_RES_LINK="-Wl,emacs.res"
   else
-    UPDATE_MANIFEST=update-game-score.exe.manifest
     W32_OBJ="$W32_OBJ w32.o w32console.o w32heap.o w32inevt.o w32proc.o"
     W32_LIBS="$W32_LIBS -lwinmm -lgdi32 -lcomdlg32"
     W32_LIBS="$W32_LIBS -lmpr -lwinspool -lole32 -lcomctl32 -lusp10"
@@ -2054,7 +2049,6 @@ AC_DEFUN
 AC_SUBST(W32_LIBS)
 AC_SUBST(EMACSRES)
 AC_SUBST(EMACS_MANIFEST)
-AC_SUBST(UPDATE_MANIFEST)
 AC_SUBST(CLIENTRES)
 AC_SUBST(CLIENTW)
 AC_SUBST(W32_RES_LINK)
@@ -5432,7 +5426,7 @@ AC_DEFUN
 dnl You might wonder (I did) why epaths.h is generated by running make,
 dnl rather than just letting configure generate it from epaths.in.
 dnl One reason is that the various paths are not fully expanded (see above);
-dnl eg gamedir=${prefix}/var/games/emacs.
+dnl e.g., gamedir='${localstatedir}/games/emacs'.
 dnl Secondly, the GNU Coding standards require that one should be able
 dnl to run 'make prefix=/some/where/else' and override the values set
 dnl by configure.  This also explains the 'move-if-change' test and
diff --git a/etc/NEWS b/etc/NEWS
index f0df0a7..cd829bf 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -65,6 +65,11 @@ emacs-version and erc-cmd-SV functions, and the leave the following
 variables nil: emacs-build-system, emacs-build-time,
 erc-emacs-build-time.
 
+** The configure option '--with-gameuser' now defaults to 'no',
+as this appears to be the most common configuration in practice.
+When it is 'no', the shared game directory and the auxiliary program
+update-game-score are no longer needed and are not installed.
+
 ** Emacs no longer works on IRIX.  We expect that Emacs users are not
 affected by this, as SGI stopped supporting IRIX in December 2013.
 
diff --git a/lib-src/Makefile.in b/lib-src/Makefile.in
index 88f6280..608b978 100644
--- a/lib-src/Makefile.in
+++ b/lib-src/Makefile.in
@@ -40,7 +40,6 @@ C_SWITCH_MACHINE=
 PROFILING_CFLAGS = @PROFILING_CFLAGS@
 WARN_CFLAGS = @WARN_CFLAGS@
 WERROR_CFLAGS = @WERROR_CFLAGS@
-UPDATE_MANIFEST = @UPDATE_MANIFEST@
 
 # Program name transformation.
 TRANSFORM = @program_transform_name@
@@ -130,9 +129,12 @@ abs_top_srcdir=
 # to '../configure'.
 archlibdir=@archlibdir@
 
+# Where to install game score files, if gameuser or gamegroup is nonempty.
 gamedir=@gamedir@
 gameuser=@gameuser@
 gamegroup=@gamegroup@
+# Nonempty if and only if a shared gamedir is used.
+use_gamedir=$(gameuser)$(gamegroup)
 
 # ==================== Utility Programs for the Build =================
 
@@ -156,13 +158,13 @@ INSTALLABLES =
 # Things that Emacs runs internally, or during the build process,
 #  which should not be installed in bindir.
 UTILITIES = profile${EXEEXT} movemail${EXEEXT} hexl${EXEEXT} \
-            update-game-score${EXEEXT}
+            $(and $(use_gamedir), update-game-score${EXEEXT})
 
 DONT_INSTALL= make-docfile${EXEEXT}
 
 # Like UTILITIES, but they're not system-dependent, and should not be
 #  deleted by the distclean target.
-SCRIPTS= rcs2log $(UPDATE_MANIFEST)
+SCRIPTS= rcs2log
 
 # All files that are created by the linker, i.e., whose names end in ${EXEEXT}.
 EXE_FILES = ${INSTALLABLES} ${UTILITIES} ${DONT_INSTALL}
@@ -258,9 +260,6 @@ maybe-blessmail:
 
 ## Install the internal utilities.  Until they are installed, we can
 ## just run them directly from lib-src.
-## If the chown/chmod commands fail, that is not a big deal.
-## update-game-score will detect at runtime that it is not setuid,
-## and handle things accordingly.
 $(DESTDIR)${archlibdir}: all
 	@echo
 	@echo "Installing utilities run internally by Emacs."
@@ -272,28 +271,22 @@ $(DESTDIR)${archlibdir}:
 	      "$(DESTDIR)${archlibdir}/$$file" || exit; \
 	  done ; \
         fi
-	umask 022 && ${MKDIR_P} "$(DESTDIR)${gamedir}" && \
+  ifneq (,$(use_gamedir))
+	umask 022 && ${MKDIR_P} "$(DESTDIR)${gamedir}"
 	touch "$(DESTDIR)${gamedir}/snake-scores" \
 	      "$(DESTDIR)${gamedir}/tetris-scores"
-ifneq ($(gameuser),)
-	if chown ${gameuser} \
-	     "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}" && \
-	   chmod u+s,go-r \
-	     "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"; \
-	then \
-	  chown ${gameuser} "$(DESTDIR)${gamedir}" && \
-	  chmod u=rwx,g=rx,o=rx "$(DESTDIR)${gamedir}"; \
-	fi
-else ifneq ($(gamegroup),)
-	if chgrp ${gamegroup} \
-	     "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}" && \
-	   chmod g+s,o-r \
-	     "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"; \
-	then \
-	  chgrp ${gamegroup} "$(DESTDIR)${gamedir}" && \
-	  chmod u=rwx,g=rwx,o=rx "$(DESTDIR)${gamedir}"; \
-	fi
-endif
+    ifneq (,$(gameuser))
+	chown ${gameuser} "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"
+	chmod u+s,go-r "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"
+	chown ${gameuser} "$(DESTDIR)${gamedir}"
+	chmod u=rwx,g=rx,o=rx "$(DESTDIR)${gamedir}"
+    else
+	chgrp ${gamegroup} "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"
+	chmod g+s,o-r "$(DESTDIR)${archlibdir}/update-game-score${EXEEXT}"
+	chgrp ${gamegroup} "$(DESTDIR)${gamedir}"
+	chmod u=rwx,g=rwx,o=rx "$(DESTDIR)${gamedir}"
+    endif
+  endif
 	exp_archlibdir=`cd "$(DESTDIR)${archlibdir}" && /bin/pwd` && \
 	if [ "$$exp_archlibdir" != "`cd ${srcdir} && /bin/pwd`" ]; then \
 	  for file in ${SCRIPTS}; do \
diff --git a/lib-src/update-game-score.exe.manifest b/lib-src/update-game-score.exe.manifest
deleted file mode 100644
index 1db836b..0000000
--- a/lib-src/update-game-score.exe.manifest
+++ /dev/null
@@ -1,10 +0,0 @@
-<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
-<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
-  <v3:trustInfo xmlns:v3="urn:schemas-microsoft-com:asm.v3">
-    <v3:security>
-      <v3:requestedPrivileges>
-	<v3:requestedExecutionLevel level="asInvoker" />
-      </v3:requestedPrivileges>
-    </v3:security>
-  </v3:trustInfo>
-</assembly>
diff --git a/lisp/play/gamegrid.el b/lisp/play/gamegrid.el
index b0ccbd3..0386a89 100644
--- a/lisp/play/gamegrid.el
+++ b/lisp/play/gamegrid.el
@@ -475,17 +475,19 @@ gamegrid-add-score
 ;;        update FILE.  This is for the case that a user has installed
 ;;        a game on her own.
 ;;
-;;     4. "update-game-score" is not setgid/setuid.  Use it to
-;;        create/update FILE in the user's home directory.  There is
-;;        presumably no shared game directory.
+;;     4. "update-game-score" does not exist or is not setgid/setuid.
+;;        Create/update FILE in the user's home directory, without
+;;        using "update-game-score".  There is presumably no shared
+;;        game directory.
 
 (defvar gamegrid-shared-game-dir)
 
 (defun gamegrid-add-score-with-update-game-score (file score)
   (let ((gamegrid-shared-game-dir
-	 (not (zerop (logand (file-modes
-			      (expand-file-name "update-game-score"
-						exec-directory))
+	 (not (zerop (logand (or (file-modes
+				  (expand-file-name "update-game-score"
+						    exec-directory))
+				  0)
 			     #o6000)))))
     (cond ((file-name-absolute-p file)
 	   (gamegrid-add-score-insecure file score))
@@ -497,23 +499,12 @@ gamegrid-add-score-with-update-game-score
 	    (expand-file-name file shared-game-score-directory) score))
 	  ;; Else: Add the score to a score file in the user's home
 	  ;; directory.
-	  (gamegrid-shared-game-dir
-	   ;; If `gamegrid-shared-game-dir' is non-nil, then
-	   ;; "update-gamescore" program is setuid, so don't use it.
-	   (unless (file-exists-p
-		    (directory-file-name gamegrid-user-score-file-directory))
-	     (make-directory gamegrid-user-score-file-directory t))
-	   (gamegrid-add-score-insecure file score
-					gamegrid-user-score-file-directory))
 	  (t
 	   (unless (file-exists-p
 		    (directory-file-name gamegrid-user-score-file-directory))
 	     (make-directory gamegrid-user-score-file-directory t))
-	   (let ((f (expand-file-name file
-				      gamegrid-user-score-file-directory)))
-	     (unless (file-exists-p f)
-	       (write-region "" nil f nil 'silent nil 'excl))
-	     (gamegrid-add-score-with-update-game-score-1 file f score))))))
+	   (gamegrid-add-score-insecure file score
+					gamegrid-user-score-file-directory)))))
 
 (defun gamegrid-add-score-with-update-game-score-1 (file target score)
   (let ((default-directory "/")
diff --git a/make-dist b/make-dist
index 41203b2..e85a2d6 100755
--- a/make-dist
+++ b/make-dist
@@ -459,7 +459,6 @@ files=
  ln [a-zA-Z]*.[ch] ../${tempdir}/lib-src
  ln ChangeLog.*[0-9] Makefile.in README ../${tempdir}/lib-src
  ln rcs2log ../${tempdir}/lib-src
- ln update-game-score.exe.manifest ../${tempdir}/lib-src)
 
 echo "Making links to 'm4'"
 (cd m4
diff --git a/src/callproc.c b/src/callproc.c
index 08fa6e9..0504857 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1584,13 +1584,14 @@ init_callproc (void)
   sh = getenv ("SHELL");
   Vshell_file_name = build_string (sh ? sh : "/bin/sh");
 
-#ifdef DOS_NT
-  Vshared_game_score_directory = Qnil;
-#else
-  Vshared_game_score_directory = build_unibyte_string (PATH_GAME);
-  if (NILP (Ffile_accessible_directory_p (Vshared_game_score_directory)))
-    Vshared_game_score_directory = Qnil;
-#endif
+  Lisp_Object gamedir = Qnil;
+  if (PATH_GAME)
+    {
+      Lisp_Object path_game = build_unibyte_string (PATH_GAME);
+      if (file_accessible_directory_p (path_game))
+	gamedir = path_game;
+    }
+  Vshared_game_score_directory = gamedir;
 }
 
 void
@@ -1661,11 +1662,6 @@ includes this.  */);
   DEFVAR_LISP ("shared-game-score-directory", Vshared_game_score_directory,
 	       doc: /* Directory of score files for games which come with GNU Emacs.
 If this variable is nil, then Emacs is unable to use a shared directory.  */);
-#ifdef DOS_NT
-  Vshared_game_score_directory = Qnil;
-#else
-  Vshared_game_score_directory = build_string (PATH_GAME);
-#endif
 
   DEFVAR_LISP ("initial-environment", Vinitial_environment,
 	       doc: /* List of environment variables inherited from the parent process.
-- 
2.9.3