Re: Cygwin: texi2dvi stumbles over texinfo.tex

[Ralf Wildenhues]

Hi Karl, Christopher,

* Karl Berry wrote on Sat, May 27, 2006 at 01:21:16AM CEST:
> 
> [Excluding cygwin, I doubt they care about this part.]

Oh yes they will care especially about this part.  ;-)

>     $ texindex ./main.au ./main.cp ./main.pg ./main.sb
>     Segmentation fault (core dumped)
> 
> Regrettably, with both the texindex in the released 4.8, and the
> texindex from current CVS, I get no seg fault on GNU/Linux (x86).

That's because the bug isn't in GNU texinfo-4.8, but in the Cygwin
package texinfo-4.8-2.

* Christopher Faylor wrote on Fri, May 26, 2006 at 07:28:13AM CEST:
> That's apparently a call to mktemp, although I can't tell what's causing
> the problem.
> 
> Setting the CYGWIN environment variable to error_start:gdb might help
> with finding what the argument to mktemp is.

Thanks for those hints.  It turns out to be mktemp.

Running under gdb (or with CYGWIN set accordingly) doesn't help much if
you don't have source and debugging symbols are not compiled in.  So I
learned that Cygwin allows to install source packages, rebuilt
texinfo-4.8-2, and also built the pristine texinfo-4.8 from gnu.org.
The latter worked fine, the former crashes.  The diff between both is
shown at the end of this message.

The obvious bug is
+  tempbase = mktemp ("txidxXXXXXX");

as mktemp changes the string it gets a pointer to, but here it gets a
pointer to potentially read-only memory.  And recent GCCs do put this in
the read-only section of the binary.  This can be rewritten to work like
this:
  {
    static char s[] = "txidxXXXXXX";
    tempbase = mktemp (s);
  }

but since the original code works fine on Cygwin, I don't see why the
change shouldn't just be dropped (in case you haven't noticed: texinfo
comes with a mkstemp replacement function, and the configure script will
cause to use that if the system doesn't have one; but also Cygwin
apparently *has* mkstemp -- at least the configure test for it succeeds
-- even though there seems to be no manpage for it).

While at it, here's a chance to reconcile the remaining differences
between the upstream and the Cygwin version:

- CVS texinfo has the findprog function in texi2dvi fixed right (i.e.,
  by using $IFS for splitting), but the script needs to initialize IFS
  to <space><tab><newline> at the beginning -- see here for why:
  http://lists.gnu.org/archive/html/automake-patches/2006-05/msg00008.html

- I don't know about the NULL_DEVICE and the path_sep setting changes
  -- you need to hash that out yourself.  (For path_sep, you could take
  a look at Autoconf's internal _AS_PATH_SEPARATOR_PREPARE macro.
  What's wrong with reusing its result, stored in PATH_SEPARATOR?)

Cheers,
Ralf

--- texinfo-4.8/lib/system.h    2004-04-26 14:56:57.000000000 +0100
+++ texinfo-4.8-2/lib/system.h  2006-04-30 03:38:39.001000000 +0100
@@ -219,6 +219,8 @@
 # ifdef __CYGWIN__
 #  define DEFAULT_TMPDIR       "/tmp/"
 #  define PATH_SEP     ":"
+#  undef NULL_DEVICE
+#  define NULL_DEVICE "/dev/null"
 # else  /* O_BINARY && !__CYGWIN__ */
 #  define DEFAULT_TMPDIR       "c:/"
 #  define PATH_SEP     ";"
--- texinfo-4.8/util/texi2dvi   2004-12-31 19:03:05.000000000 +0100
+++ texinfo-4.8-2/util/texi2dvi 2005-04-12 02:47:12.001000000 +0100
@@ -1,4 +1,4 @@
-#! /bin/sh
+#! /bin/bash
 # texi2dvi --- produce DVI (or PDF) files from Texinfo (or LaTeX) sources.
 # $Id: texi2dvi,v 1.34 2004/12/01 18:35:36 karl Exp $
 #
@@ -101,13 +101,7 @@
 
 orig_pwd=`pwd`
 
-# Systems which define $COMSPEC or $ComSpec use semicolons to separate
-# directories in TEXINPUTS.
-if test -n "$COMSPEC$ComSpec"; then
-  path_sep=";"
-else
-  path_sep=":"
-fi
+path_sep=":"
 
 # Pacify verbose cds.
 CDPATH=${ZSH_VERSION+.}$path_sep
@@ -118,14 +112,8 @@
 # return true if program $1 is somewhere in PATH, else false.
 #
 findprog () {
-  foundprog=false
-  for dir in `echo $PATH | tr "$path_sep" " "`; do
-    if test -x "$dir/$1"; then  # does anyone still need test -f?
-      foundprog=true
-      break
-    fi
-  done
-  $foundprog
+  /usr/bin/which "$1" >/dev/null 2>&1
+  return $?
 }
 
 # Report an error and exit with failure.
--- texinfo-4.8/util/texindex.c 2004-04-11 18:56:47.000000000 +0100
+++ texinfo-4.8-2/util/texindex.c       2005-10-07 15:43:50.001000000 +0100
@@ -99,6 +99,9 @@
 /* Directory to use for temporary files.  On Unix, it ends with a slash.  */
 char *tempdir;
 
+/* Basename for temp files inside of tempdir.  */
+char *tempbase;
+
 /* Number of last temporary file.  */
 int tempcount;
 
@@ -190,6 +193,11 @@
 
   decode_command (argc, argv);
 
+  /* XXX mkstemp not appropriate, as we need to have somewhat predictable
+   * names. But race condition was fixed, see maketempname. 
+   */
+  tempbase = mktemp ("txidxXXXXXX");
+
   /* Process input files completely, one by one.  */
 
   for (i = 0; i < num_infiles; i++)
@@ -389,21 +397,21 @@
 static char *
 maketempname (int count)
 {
-  static char *tempbase = NULL;
   char tempsuffix[10];
-
-  if (!tempbase)
-    {
-      int fd;
-      tempbase = concat (tempdir, "txidxXXXXXX");
-
-      fd = mkstemp (tempbase);
-      if (fd == -1)
-        pfatal_with_name (tempbase);
-    }
+  char *name, *tmp_name;
+  int fd;
 
   sprintf (tempsuffix, ".%d", count);
-  return concat (tempbase, tempsuffix);
+  tmp_name = concat (tempdir, tempbase);
+  name = concat (tmp_name, tempsuffix);
+  free(tmp_name);
+
+  fd = open (name, O_CREAT|O_EXCL|O_WRONLY, 0600);
+  if (fd == -1)
+    pfatal_with_name (name);
+
+  close(fd);
+  return name;
 }