Re: [PATCH 1/2] Fix detection of GNU M4 on mksh
Hi/2. Zack Weinberg wrote: > On Sun, Oct 15, 2023, at 2:58 AM, KO Myung-Hun wrote: >> Zack Weinberg wrote: >>> On Sat, Oct 14, 2023, at 9:19 AM, KO Myung-Hun wrote: * m4/m4.m4 (AC_PROG_GNU_M4): Double-quotes $ac_snip2. >>> Please explain why it isn't also necessary to add double quotes >>> around all the other variable expansions in this check, particularly >>> $ac_snippet and $ac_path_M4. >> >> Because this test fails due to $ac_snip2 > > Details please? > Commit message was not enough ? $ac_snip2 contains NL. And in order to print it, `print -r --' command is used on mksh. BTW, printing without double-quoting, NL in $ac_snip2 is replaced with a space. Because of this, M4 does not return contents expected. As a result, test for $ac_snip2 fails. -- KO Myung-Hun Korean OS/2 User Community : https://www.os2.kr/
Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2
Hi/2. Zack Weinberg wrote: > On Sun, Oct 15, 2023, at 3:43 AM, KO Myung-Hun wrote: >> Zack Weinberg wrote: >>> On Sat, Oct 14, 2023, at 9:19 AM, KO Myung-Hun wrote: * bin/autom4te.in (handle_output): Ignore setting mode failure on OS/2. >>> >>> Not OK, for two reasons: > ... >> How about this ? >> 1. create and close a temporary file >> 2. chmod() on it >> 3. re-open it with O_TRUNC ? > > Multi-user security is probably not a concern for modern-day use of > OS/2. Also, the temporary files created by the code we’re talking > about are not created in a system-wide scratch directory. So we > probably *could* do it this way, but I don’t like it, because it’s > not safe in the general case. > > The trouble is, on a multi-user system, any time you do any operation > by name on a file whose full pathname includes a world-writable > directory (such as the system-wide scratch directories), even if that > directory is “sticky” (chmod +t), you have to be exquisitely careful, > or a malicious concurrent process might be able to trick you into > overwriting some file elsewhere on the filesystem. For example, your > steps 2 and 3, if executed as root on a file expected to exist in > /tmp, would give a malicious concurrent process a chance to clobber > the access control bits and/or the contents of *any file*, by moving > the temporary file out of the way, and replacing it with a symlink, > in between steps 1 and 2. That’s a narrow race window to hit, but > it has been done successfully in the past. > > I don’t want to have code in Autoconf that is only safe because of > non-obvious details of the context it’s used in. People might > reuse the code in a different context where it’s *not* safe, without > realizing the danger. So I suggest we use the appended patch > instead of your patch. I’ve tested this on Unix systems with both > Perl 5.6 and Perl 5.38. Could you please test it on your OS/2 system > as well? > It works well here. Thanks! -- KO Myung-Hun Korean OS/2 User Community : https://www.os2.kr/
Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2
On Tue, Oct 17, 2023, at 2:58 PM, Paul Eggert wrote: > On 10/17/23 11:16, Zack Weinberg wrote: ... >> you have to be exquisitely careful, or a malicious concurrent process >> might be able to trick you into overwriting some file elsewhere on >> the filesystem. ... > ? If /tmp is sticky, a malicious process can't rename /tmp/foo. I might be wrong about that specific thing. It's been long enough that I no longer remember the exact details, but there was a CVE reported against GCC ... I want to say circa version 2.95 ... because it would create temporary files with predictable names in /tmp and it was *somehow* possible for a malicious process to substitute symlinks pointing into /etc, and if you were running the compiler as root, which you shouldn't but it happens all the time, boom, trashed /etc/shadow or something equally important. It is possible that this exploit depended on a kernel bug where the sticky bit didn't do everything it needed to do, but since people still want to run autoconf proper (not just configure scripts) on ancient systems, I think we need to be careful anyway. zw
Re: [PATCH 2/2] Ignore failure of setting mode on a temporary file on OS/2
On Wed, Oct 18, 2023, at 10:21 AM, KO Myung-Hun wrote:
> Zack Weinberg wrote:
>> I don’t want to have code in Autoconf that is only safe because of
>> non-obvious details of the context it’s used in. People might
>> reuse the code in a different context where it’s *not* safe, without
>> realizing the danger. So I suggest we use the appended patch
>> instead of your patch. I’ve tested this on Unix systems with both
>> Perl 5.6 and Perl 5.38. Could you please test it on your OS/2 system
>> as well?
>
> It works well here.
Great! I have committed the version below.
zw
>From c4a695510d240491f89d78204a3d5a6fdbe03648 Mon Sep 17 00:00:00 2001
From: Zack Weinberg
Date: Wed, 18 Oct 2023 13:23:36 -0400
Subject: [PATCH] autom4te: OS/2 compat: Do not attempt to chmod an open file.
On OS/2, chmod(2) cannot be applied to an open file.
Instead set the desired permissions when the file is initially
created, using the PERMS argument to File::Temp::tempfile if
possible, or by manually emulating that feature if the system
perl does not provide a new enough version of File::Temp.
This has the nice side effect that we no longer need to handle
the umask manually.
* autom4te.in (tempfile_with_mode): New function.
(handle_output): Use tempfile_with_mode instead of directly using
File::Temp plus chmod.
Co-authored-by: KO Myung-Hun
---
bin/autom4te.in | 56 +
1 file changed, 47 insertions(+), 9 deletions(-)
diff --git a/bin/autom4te.in b/bin/autom4te.in
index 71d7e6a6..41da77b0 100644
--- a/bin/autom4te.in
+++ b/bin/autom4te.in
@@ -222,6 +222,52 @@ Written by Akim Demaille.
## Routines. ##
## -- ##
+# tempfile_with_mode ($dir, $mode)
+#
+# Create a temporary file in $dir with access control bits $mode.
+# Returns a list ($fh, $fname) where $fh is a filehandle open for
+# writing to the file, and $fname is the name of the file.
+sub tempfile_with_mode ($$)
+{
+ my ($dir, $mode) = @_;
+
+ require File::Temp;
+ my $template = "actmp." . File::Temp::TEMPXXX;
+
+ # The PERMS argument was added to File::Temp::tempfile in version
+ # 0.2310 of the File::Temp module; it will be silently ignored if
+ # passed to an older version of the function. This is the simplest
+ # way to do a non-fatal version check without features of Perl 5.10.
+ local $@;
+ if (eval { File::Temp->VERSION("0.2310"); 1 })
+{
+ # Can use PERMS argument to tempfile().
+ return File::Temp::tempfile ($template, DIR => $dir, PERMS => $mode,
+ UNLINK => 0);
+}
+ else
+{
+ # PERMS is not available.
+ # This is functionally equivalent to what it would do.
+ require Fcntl;
+ my $openflags = Fcntl::O_RDWR | Fcntl::O_CREAT | Fcntl::O_EXCL;
+
+ require File::Spec;
+ $template = File::Spec->catfile($dir, $template);
+
+ # 50 = $MAX_GUESS in File::Temp (not an exported constant).
+ for (my $i = 0; $i < 50; $i++)
+{
+ my $filename = File::Temp::mktemp($template);
+ my $fh;
+ my $success = sysopen ($fh, $filename, $openflags, $mode);
+ return ($fh, $filename) if $success;
+ fatal "Could not create temp file $filename: $!"
+unless $!{EEXIST};
+}
+ fatal "Could not create any temp file from $template: $!";
+}
+}
# $OPTION
# files_to_options (@FILE)
@@ -563,15 +609,7 @@ sub handle_output ($$)
else
{
my (undef, $outdir, undef) = fileparse ($output);
-
- use File::Temp qw (tempfile);
- ($out, $scratchfile) = tempfile (UNLINK => 0, DIR => $outdir);
- fatal "cannot create a file in $outdir: $!"
-unless $out;
-
- # File::Temp doesn't give us access to 3-arg open(2), unfortunately.
- chmod (oct ($mode) & ~(umask), $scratchfile)
-or fatal "setting mode of " . $scratchfile . ": $!";
+ ($out, $scratchfile) = tempfile_with_mode ($outdir, oct($mode));
}
my $in = new Autom4te::XFile ($ocache . $req->id, "<");
--
2.41.0
Re: [PATCH 1/2] Fix detection of GNU M4 on mksh
On Wed, Oct 18, 2023, at 10:09 AM, KO Myung-Hun wrote:
> Zack Weinberg wrote:
>> Details please?
>
> Commit message was not enough ?
I'm afraid not. I needed to know everything you say in the next two
paragraphs:
> $ac_snip2 contains NL. And in order to print it, `print -r --' command
> is used on mksh.
>
> BTW, printing without double-quoting, NL in $ac_snip2 is replaced with
> a space. Because of this, M4 does not return contents expected. As a
> result, test for $ac_snip2 fails.
With this information I can understand your suggested change easily;
without it I thought I might have to construct my own OS/2+mksh
environment in order to figure out why this test was failing *only* in
that environment.
The reason this test is failing only in your environment, is because
you generated the configure script *for Autoconf itself* with an older
version of Autoconf. Current versions do not use `print -r` in the
implementation of AS_ECHO. If you are building Autoconf from a git
checkout, you should use the "bootstrap" script in the source directory
to generate the configure script; if you are building from a release
tarball you should use the configure script included in that tarball.
Either of these should have masked the bug you found.
Having said that, you *have* found a genuine bug. Autoconf's
documentation is quite clear that the argument to AS_ECHO must be a
"single shell word", in particular, not containing any unquoted
whitespace (not just no newlines) after variable expansion. *Both*
$ac_snippet and $ac_snip2 contain whitespace and therefore need to be
quoted. It happens that replacing newlines with horizontal spaces
breaks $ac_snip2 but not $ac_snippet, and it also happens that the
`printf`-based implementation of AS_ECHO (which is used unconditionally
by current Autoconf) mangles both in a different way that doesn't break
either of them. But we should not be relying on either of these things.
So I'm going to go ahead and commit your patch with quoting added to
both $ac_snip2 and $ac_snippet. Final patch below.
zw
>From 88011e1f263fd7e794caa6e0984e623769d1c8c3 Mon Sep 17 00:00:00 2001
From: KO Myung-Hun
Date: Wed, 18 Oct 2023 14:00:11 -0400
Subject: [PATCH] m4/m4.m4: Quote argument to AS_ECHO correctly.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
AS_ECHO’s argument is required to be “a single shell word,”
and this more precisely means it must not be altered by shell
word splitting. In particular, if the argument contains shell
variables whose values contain whitespace then it needs to be
wrapped in "shell double quotes".
The absence of these quotes caused one of the embedded M4 scripts
to be mangled by the Autoconf 2.69 implementation of AS_ECHO.
We don’t officially support bootstrapping with an older version
of Autoconf (use the ./bootstrap script instead) but the absence
of quotes is still incorrect.
For consistency add [M4 quotes] to the use of AS_ECHO that was
shell-quoted but not M4-quoted.
* m4/m4.m4 (AC_PROG_GNU_M4): Quote argument to AS_ECHO correctly.
---
m4/m4.m4 | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/m4/m4.m4 b/m4/m4.m4
index e6203118..919dbf4a 100644
--- a/m4/m4.m4
+++ b/m4/m4.m4
@@ -41,12 +41,12 @@ AC_PATH_PROGS_FEATURE_CHECK([M4], [m4 gm4 gnum4],
ac_snip2=change'quote(<,>)def''ine(,<>)d'nl
ac_snip2=${ac_snip2}${as_nl}def'ine(,)>)d'nl
ac_snip2=${ac_snip2}${as_nl}m4'wrap()d'nl
- AS_ECHO("$as_me:${as_lineno-$LINENO}: trying $ac_path_M4") \
+ AS_ECHO(["$as_me:${as_lineno-$LINENO}: trying $ac_path_M4"]) \
>&AS_MESSAGE_LOG_FD
test -z "`$ac_path_M4 -F conftest.m4f &1`" \
- && test -z "`AS_ECHO([$ac_snippet]) | $ac_path_M4 --trace=mac 2>&1`" \
+ && test -z "`AS_ECHO(["$ac_snippet"]) | $ac_path_M4 --trace=mac 2>&1`" \
&& test -f conftest.m4f \
- && test x"`AS_ECHO([$ac_snip2]) | \
+ && test x"`AS_ECHO(["$ac_snip2"]) | \
$ac_path_M4 --trace=T --debug=aflq 2>&1`" = \
x'm4trace:stdin:3: -1- T()' \
&& ac_cv_path_M4=$ac_path_M4 ac_path_M4_found=:
--
2.41.0
