bug#7928: mktime test in configure: UB resulting in infinite loop
The configure test for mktime (m4/mktime.m4) contains the following
code:
for (;;)
{
t = (time_t_max << 1) + 1;
if (t <= time_t_max)
break;
time_t_max = t;
}
This code has undefined behavior on signed integer overflow; at least
some versions of gcc, and any sane compiler, will optimize out the
exit condition since algebraically 2x+1>x for any nonnegative x. The
result is an infinite loop and failure of the test after the 60-second
timeout.
Finding the max possible value for a signed integer type is actually a
very hard problem in C. As far as I know it's impossible at
compile-time and might even be impossible at runtime unless you make
some assumptions (either the absence of padding bits, or the
well-definedness of converting larger/unsigned types to signed types).
The approach I would take is just:
time_t_max = (time_t)1 << 8*sizeof(time_t)-2;
If this test comes from higher-up (gnulib?) please forward my bug
report to the relevant upstream.
bug#7928: mktime test in configure: UB resulting in infinite loop
On Thu, Jan 27, 2011 at 08:14:56AM -0700, Eric Blake wrote: > # define TYPE_MINIMUM(t) \ > ((t) (! TYPE_SIGNED (t) \ > ? (t) 0 \ > : TYPE_SIGNED_MAGNITUDE (t) \ > ? ~ (t) 0 \ > : ~ (t) 0 << (sizeof (t) * CHAR_BIT - 1))) > # define TYPE_MAXIMUM(t) \ > ((t) (! TYPE_SIGNED (t) \ > ? (t) -1 \ > : ~ (~ (t) 0 << (sizeof (t) * CHAR_BIT - 1 The last line of this macro has UB due to signed integer overflow in the << operation. Replace it with (( (t)1 << CHAR_BIT*sizeof(time_t)-2 ) - 1) * 2 + 1 Which for a 32-bit type would expand as: (0x4000 - 1) * 2 + 1 0x3fff *2 + 1 0x7ffe + 1 0x7fff With no overflows. > and no one has complained yet, so we might as well just use this same > logic in m4/mktime.m4. Well apparently no one complained about the overflow in coreutils either. Perhaps later gcc versions are more forgiving; I found it building on a system where I have to use gcc 3.2.3, which, being one of the earlier versions to utilize the UB of signed overflow, might be less diplomatic about it. Anyway to avoid future trouble, I would strive to remove all signed overflow UB from the tests even if it doesn't presently hurt anyone. > 8 is a magic number; it would be better to use CHAR_BIT, as was done in > intprops.h. As you wish. This code (coreutils) is sufficiently POSIX-like-system dependent that I thought using POSIX's requirement CHAR_BIT==8 was reasonable. > > If this test comes from higher-up (gnulib?) please forward my bug > > report to the relevant upstream. > > Forwarded; and the patch should be applied shortly. Thanks! Rich
bug#7928: mktime test in configure: UB resulting in infinite loop
On Thu, Jan 27, 2011 at 07:42:10PM +0100, Bruno Haible wrote: > Do you mean to say that GCC produces undefined behaviour for shifts of > negative values, even those where the result is negative (no overflow)? > I've never seen a sign of that. I mean to say that left-shifting a negative value *at all* is undefined behavior. I doubt gcc will ever break it, but why not use my version of the code that's 100% safe and never invokes undefined behavior? Rich
bug#7928: mktime test in configure: UB resulting in infinite loop
On Thu, Jan 27, 2011 at 11:42:25PM -0800, Paul Eggert wrote: > On 01/27/2011 02:08 PM, Rich Felker wrote: > > I mean to say that left-shifting a negative value *at all* is > > undefined behavior. I doubt gcc will ever break it, but why not use my > > version of the code that's 100% safe and never invokes undefined > > behavior? > > Your version of the code provokes similar undefined behavior > when computing TYPE_MINIMUM, which means that overall it's > no more reliable than what's there now. An advantage of > the current approach is that there's a clear relationship > between TYPE_MINIMUM and TYPE_MAXIMUM, and this aids understanding. > If it could be done just as clearly by other means, that would > be OK too. My version only computes the maximum. The minimum is -max if the type is sign/magnitude or ones complement and -max-1 if the type is twos complement. Testing which of the three allowable signed integer representations is used is easy: compare ~(t)1 against and -(t)1 and -(t)2. Rich
bug#7928: mktime test in configure: UB resulting in infinite loop
On Fri, Jan 28, 2011 at 06:57:22PM +0100, Bruno Haible wrote: > Rich Felker wrote: > > Testing which of the three allowable signed integer > > representations is used is easy: compare ~(t)1 against and -(t)1 and > > -(t)2. > > Testing which of the three signed integer representations is in use > is not even needed: Your formula > t) 1 << (sizeof (t) * CHAR_BIT - 2)) - 1) * 2 + 1) > yields the correct result in all three cases. It yields the correct max. However, getting the min requires knowing whether min is -max or -max-1. This depends on which of the representations is used: min = ~(t)1 == -(t)2 ? -max-1 : -max; Rich
bug#17196: UTF-8 printf string formating problem
On Wed, Apr 09, 2014 at 02:49:37PM +0200, Steffen Nurpmeso wrote: > Eric Blake wrote: > |>> Dan Douglas wrote: > |>>> ksh93 already has this feature using the "L" modifier: > |>>> > |>>> ksh -c "printf '%.3Ls\n' $'\u2605\u2605\u2605\u2605\u2605'" > |>>> ★★★ > |>> > |>> At least there is prior art for it. > |> > |> So we can count bytes, chars or cells (graphemes). > |> > |> Thinking a bit more about it, I think shell level printf > |> should be dealing in text of the current encoding and counting cells. > |> In the edge case where you want to deal in bytes one can do: > |> LC_ALL=C printf ... > |> > |> I see that ksh behaves as I would expect and counts cells, > |> though requires the explicit %L enabler: > |> $ ksh -c "printf '%.3Ls\n' $'a\u0301\u2605\u2605\u2605'" > |> á★★ > |> $ ksh -c "printf '%.3Ls\n' $'Ａ\u2605\u2605\u2605'" > |> Ａ★ > |> $ ksh -c "printf '%.3Ls\n' $'ＡＡ\u2605\u2605\u2605'" > |> Ａ > |> > |> zsh seems to just count characters: > |> $ zsh -c "printf '%.3Ls\n' $'a\u0301\u2605\u2605\u2605'" > |> á★ > |> $ zsh -c "printf '%.3s\n' $'a\u0301\u2605\u2605\u2605'" > |> á★ > |> $ zsh -c "printf '%.3Ls\n' $'Ａ\u2605\u2605\u2605'" > |> Ａ★★ > |> > |> I see that dash gives invalid directive for any of %ls %Ls %S. > |> > |> Pity there is no consensus here. > |> Personally I would go for: > |> printf '%3s' 'blah' # count cells > |> printf '%3Ls' 'blah' # count chars > |> LANG=C '%3Ls' 'blah' # count bytes > |> LANG=C '%3s' 'blah' # count bytes > | > |Hmm. POSIX requires support for %ls (aka %S) according to byte counts, > |and currently states that %Ls is undefined. But I would LOVE to have a > |standardized spelling for counting characters instead of bytes. The > |extension %Ls looks like a good candidate for standardization, precisely > |because counting characters when printing a multibyte string is more > |useful than counting bytes (you do NOT want to end in the middle of a > |multibyte character), and because ksh offers it as existing practice. > | > |Your idea for counting "cells" (by which I'm assuming you mean one or > |more characters that all display within the same cell of the terminal, > |as if the end user saw only one grapheme), on the other hand, does not > |seem to have any precedence, and I would strongly object to having %s > |count by cells because %s already has a standardized (if unfortunate) > |meaning of counting by bytes. Maybe yet another extension is warranted > |(perhaps %LLs?) as a new notion for counting by cells instead of > |characters, but it's harder to justify that without existing practice. > > I see you are trying to invent the word character for code points > and reserve the term "graphem" for user-perceived characters. > This goes in line with the GNU library which has the existing > practice to let wcwidth(3) return the value 1 for accents and > other combining code points as well as so-called (Unicode) > noncharacters. And who would call wcwidth(3) on something that is > not to be drawn onto the screen directly afterwards. And, of > course, which terminal will perform the composition of code points > written via STD I/O to characters on its own. > I think for quite a while it is up to the input methods to combine > into something precomposed in order to let POSIX programs finally > work with it. Many languages do not have precomposed forms for all the character sequences they need, and for some, it would not even be practical to have precomposed forms, and would force the use of complex input methods instead of simple keyboard maps. Rich
bug#39237: coreutils cp mishandles error return from lchmod
cp (and perhaps other utilities) are treating EOPNOTSUPP from lchmod as a hard error rather than an indication that the system does not support modes for symlinks. This recently came up on https://bugs.gentoo.org/687236#c17 where users are claiming recent changes to gnulib made the problem go away, but I'm not sure what the mechanism was, since the underlying problem is still there. Users only hit the problem on cross-compiling, presumably due to logic in how gnulib replaces lchmod, and due to gnulib's replacement wrongly following symlinks (it just #defines it to chmod). gnulib documents that the caller must check before calling lchmod that the file is not a symlink, but this is unsafe in the presence of race conditions, While lchmod is not a standard function, fchmodat with AT_SYMLINK_NOFOLLOW is, and specifies EOPNOTSUPP for the case where the system does not support modes on symlinks. musl provides lchmod as a simple wrapper for this, yielding a version that is safe. coreutils should be opting to use the system-provided lchmod, which is safe, and correctly handling error returns (silently treating EOPNOTSUPP as success) rather than as hard errors. Rich
bug#39236: [musl] coreutils cp mishandles error return from lchmod
On Wed, Jan 22, 2020 at 04:08:26PM +0100, Florian Weimer wrote: > * Rich Felker: > > > On Wed, Jan 22, 2020 at 03:34:18PM +0100, Florian Weimer wrote: > >> * Rich Felker: > >> > >> > coreutils should be opting to use the system-provided lchmod, which is > >> > safe, and correctly handling error returns (silently treating > >> > EOPNOTSUPP as success) rather than as hard errors. > >> > >> glibc's lchmod always returns ENOSYS (except on Hurd). I don't know how > >> lchmod is used in coreutils, but I suspect it is not particularly > >> useful. > > > > When preserving permissions (cp -p, archive extraction, etc.), you > > want lchmod to work correctly just for the purpose of *not* following > > the link and thereby unwantedly changing the permissions of the link > > target. But, fchmodat with AT_SYMLINK_NOFOLLOW works just as well and > > is standard, and that's really what coreutils should be using. > > I think you misread what I wrote: lchmod *always* returns ENOSYS. Even > if the file is not a symbolic link. Likewise, fchmodat with > AT_SYMLINK_NOFOLLOW *always* returns ENOTSUP. Yes, I understood that. I was going into why there should be a real implementation, but didn't make it clear that that was what I was doing. > The reason for this is that the kernel does not provide a suitable > system call to implement this, even though some file systems allow a > mode change for symbolic links. I think we can do better, although I > should note that each time we implement such emulation in userspace, it > comes back to bite us eventually. Emulations in userspace that are approximate, have race conditions, etc. are bad. Ones that are rigorous are good, though. Rich
bug#39236: [musl] coreutils cp mishandles error return from lchmod
On Wed, Jan 22, 2020 at 03:34:18PM +0100, Florian Weimer wrote: > * Rich Felker: > > > coreutils should be opting to use the system-provided lchmod, which is > > safe, and correctly handling error returns (silently treating > > EOPNOTSUPP as success) rather than as hard errors. > > glibc's lchmod always returns ENOSYS (except on Hurd). I don't know how > lchmod is used in coreutils, but I suspect it is not particularly > useful. When preserving permissions (cp -p, archive extraction, etc.), you want lchmod to work correctly just for the purpose of *not* following the link and thereby unwantedly changing the permissions of the link target. But, fchmodat with AT_SYMLINK_NOFOLLOW works just as well and is standard, and that's really what coreutils should be using. Rich
bug#39236: [musl] coreutils cp mishandles error return from lchmod
On Wed, Jan 22, 2020 at 04:32:45PM +0100, Florian Weimer wrote: > * Rich Felker: > > > On Wed, Jan 22, 2020 at 04:08:26PM +0100, Florian Weimer wrote: > >> * Rich Felker: > >> > >> > On Wed, Jan 22, 2020 at 03:34:18PM +0100, Florian Weimer wrote: > >> >> * Rich Felker: > >> >> > >> >> > coreutils should be opting to use the system-provided lchmod, which is > >> >> > safe, and correctly handling error returns (silently treating > >> >> > EOPNOTSUPP as success) rather than as hard errors. > >> >> > >> >> glibc's lchmod always returns ENOSYS (except on Hurd). I don't know how > >> >> lchmod is used in coreutils, but I suspect it is not particularly > >> >> useful. > >> > > >> > When preserving permissions (cp -p, archive extraction, etc.), you > >> > want lchmod to work correctly just for the purpose of *not* following > >> > the link and thereby unwantedly changing the permissions of the link > >> > target. But, fchmodat with AT_SYMLINK_NOFOLLOW works just as well and > >> > is standard, and that's really what coreutils should be using. > >> > >> I think you misread what I wrote: lchmod *always* returns ENOSYS. Even > >> if the file is not a symbolic link. Likewise, fchmodat with > >> AT_SYMLINK_NOFOLLOW *always* returns ENOTSUP. > > > > Yes, I understood that. I was going into why there should be a real > > implementation, but didn't make it clear that that was what I was > > doing. > > Ah, yes, there should be a real implementation if we can get full > lchmod/AT_SYMLINK_NOFOLLOW behavior on file systems that support it. If > we can't, I'm not sure if there is a point to it. The point is to fail when the target is a symlink, rather than (erroneously and possibly dangerously) applying the chmod to the link target. Actually supporting link modes is useless. It's the "not modifying the target" that's important. > >> The reason for this is that the kernel does not provide a suitable > >> system call to implement this, even though some file systems allow a > >> mode change for symbolic links. I think we can do better, although I > >> should note that each time we implement such emulation in userspace, it > >> comes back to bite us eventually. > > > > Emulations in userspace that are approximate, have race conditions, > > etc. are bad. Ones that are rigorous are good, though. > > Is there a reason for the S_ISLNK check in the musl implementation? > With current kernels, chmod on the proc pseudo-file will not traverse > the symbolic link, but I have yet to check if this has always been the > case. It's explained in the bz you just replied on, https://sourceware.org/bugzilla/show_bug.cgi?id=14578 The point of the S_ISLNK check is to fail out early with the ENOTSUPP, which the caller should treat as "success-like", in the non-racing condition, without the need to open a fd (which may fail with ENFILE/EMFILE) and without the need for /proc to be mounted. Otherwise, a different error will be produced when one of those cases is hit, and the caller will treat it as a real error. Rich
bug#39236: [musl] coreutils cp mishandles error return from lchmod
On Wed, Jan 22, 2020 at 05:19:05PM +0100, Florian Weimer wrote: > * Rich Felker: > > > On Wed, Jan 22, 2020 at 04:32:45PM +0100, Florian Weimer wrote: > >> * Rich Felker: > >> > >> > On Wed, Jan 22, 2020 at 04:08:26PM +0100, Florian Weimer wrote: > >> >> * Rich Felker: > >> >> > >> >> > On Wed, Jan 22, 2020 at 03:34:18PM +0100, Florian Weimer wrote: > >> >> >> * Rich Felker: > >> >> >> > >> >> >> > coreutils should be opting to use the system-provided lchmod, > >> >> >> > which is > >> >> >> > safe, and correctly handling error returns (silently treating > >> >> >> > EOPNOTSUPP as success) rather than as hard errors. > >> >> >> > >> >> >> glibc's lchmod always returns ENOSYS (except on Hurd). I don't know > >> >> >> how > >> >> >> lchmod is used in coreutils, but I suspect it is not particularly > >> >> >> useful. > >> >> > > >> >> > When preserving permissions (cp -p, archive extraction, etc.), you > >> >> > want lchmod to work correctly just for the purpose of *not* following > >> >> > the link and thereby unwantedly changing the permissions of the link > >> >> > target. But, fchmodat with AT_SYMLINK_NOFOLLOW works just as well and > >> >> > is standard, and that's really what coreutils should be using. > >> >> > >> >> I think you misread what I wrote: lchmod *always* returns ENOSYS. Even > >> >> if the file is not a symbolic link. Likewise, fchmodat with > >> >> AT_SYMLINK_NOFOLLOW *always* returns ENOTSUP. > >> > > >> > Yes, I understood that. I was going into why there should be a real > >> > implementation, but didn't make it clear that that was what I was > >> > doing. > >> > >> Ah, yes, there should be a real implementation if we can get full > >> lchmod/AT_SYMLINK_NOFOLLOW behavior on file systems that support it. If > >> we can't, I'm not sure if there is a point to it. > > > > The point is to fail when the target is a symlink, rather than > > (erroneously and possibly dangerously) applying the chmod to the link > > target. Actually supporting link modes is useless. It's the "not > > modifying the target" that's important. > > The kernel supports it on some file systems, though: > > $ ls -l /tmp/x > l-. 1 fweimer fweimer 6 Jan 22 15:27 /tmp/x -> /tmp/x > > Although mode 0 curiously does not prevent readlink calls. > > > It's explained in the bz you just replied on, > > https://sourceware.org/bugzilla/show_bug.cgi?id=14578 > > > > The point of the S_ISLNK check is to fail out early with the ENOTSUPP, > > which the caller should treat as "success-like", in the non-racing > > condition, without the need to open a fd (which may fail with > > ENFILE/EMFILE) and without the need for /proc to be mounted. > > Otherwise, a different error will be produced when one of those cases > > is hit, and the caller will treat it as a real error. > > Hmm. The way I read the musl code, the O_PATH descriptor already > exists. At this point, you can just chmod the O_PATH descriptor, and > have the kernel report EOPNOTSUPP if the file system does not support > that. Oh, you mean the second one after it's already open? Maybe that's ok. I was concerned it might follow the link and chmod the target at that point. I thought you were asking about the ealier check before the O_PATH open. Rich
bug#39236: [musl] coreutils cp mishandles error return from lchmod
On Wed, Jan 22, 2020 at 09:48:14PM +0100, Florian Weimer wrote: > * Rich Felker: > > >> Hmm. The way I read the musl code, the O_PATH descriptor already > >> exists. At this point, you can just chmod the O_PATH descriptor, and > >> have the kernel report EOPNOTSUPP if the file system does not support > >> that. > > > > Oh, you mean the second one after it's already open? Maybe that's ok. > > Yes, that's what I meant. > > > I was concerned it might follow the link and chmod the target at that > > point. > > In my tests, it works. I think it's also documented behavior for chown > on these pseudo-files. Do you know where we might find that documentation? > I also verified that closing an O_PATH descriptor does not release POSIX > advisory locks for the same file. But I'm wondering if there's still > something we are missing. Thanks, I hadn't thought to check that, but wouldn't have expected it to be a problem since O_PATH is not actually open to the file. Rich
bug#39236: [musl] coreutils cp mishandles error return from lchmod
On Wed, Jan 22, 2020 at 01:55:57PM -0800, Paul Eggert wrote: > On 1/22/20 7:08 AM, Florian Weimer wrote: > >I think you misread what I wrote: lchmod*always* returns ENOSYS. Even > >if the file is not a symbolic link. Likewise, fchmodat with > >AT_SYMLINK_NOFOLLOW *always* returns ENOTSUP. > > That's too bad, because coreutils (and many other applications, I > expect) assume that lchmod (and fchmodat with AT_SYMLINK_NOFOLLOW) > to act like chmod except not follow symlinks, in order to make it > less likely that the application will run afoul of a symlink race > and chmod the wrong file. Isn't that how the Linux fstatat call > behaves? And if so, why does glibc fstatat refuse to support this > behavior? I think you're confusing fchmodat with fstatat. The Linux fchmodat syscall lacks a flags argument and thus doesn't suffice to implement fchmodat. The fstatat syscall does work. > To work around this bug, I suppose coreutils etc. should do > something like the following: > > 1. Never use lchmod since the porting nightmare is bad enough without it. > > 2. On non-glibc systems (or glibc systems where the bug is fixed), > use fchmodat with AT_SYMLINK_NOFOLLOW. > > 3. On glibc systems with the bug, use openat with > AT_SYMLINK_NOFOLLOW and O_PATH, and then fchmod the resulting file > descriptor. > > Does this sound right? Or is there some O_PATH gotcha that I haven't > thought about? I think fchmod historically did not work on O_PATH file descriptors, which is why musl is using chmod on the procfs magic symlink. However, fchmodat might work too with an empty pathname; I'm not sure. I think these fixes are better encapsulated as a replacement for missing/broken fchmodat, rather than putting the logic in individual utilities or coreutils-specific library code. Also, note that if you want to skip checking stat to make sure you didn't open a symlink with O_PATH, that depends on confirming Florian's claim that the kernel documents it will not follow the symlink. > Come to think of it, perhaps the best thing would be to change > Gnulib's lchmod and fchmodat modules so that they do what > applications expect, even on buggy glibc systems. (Which would be > ironic, since Gnulib's main goal is to put wrappers around other > libraries so that they look more like glibc.) I think we're approaching a consensus that glibc should fix this too, so then it would just be gnulib matching the fix. Rich
bug#39236: [musl] coreutils cp mishandles error return from lchmod
On Wed, Feb 12, 2020 at 12:50:19PM +0100, Florian Weimer wrote: > * Paul Eggert: > > > On 1/22/20 2:05 PM, Rich Felker wrote: > >> I think we're approaching a consensus that glibc should fix this too, > >> so then it would just be gnulib matching the fix. > > > > I installed the attached patch to Gnulib in preparation for the upcoming > > glibc fix. The patch causes fchmodat with AT_SYMLINK_NOFOLLOW to work on > > non-symlinks, and similarly for lchmod on non-symlinks. The idea is to > > avoid this sort of problem in the future, and to let Coreutils etc. work > > on older platforms as if glibc 2.32 (or whatever) is already in place. > > The lchmod implementation based on /proc tickles an XFS bug: > > <https://sourceware.org/ml/libc-alpha/2020-02/msg00467.html> Uhg, why does Linux even let the fs driver see whether the chmod is being performed via a filename, O_PATH fd, or magic symlink in /proc? It should just be an operation on the inode. Rich
bug#39236: [musl] coreutils cp mishandles error return from lchmod
On Wed, Feb 12, 2020 at 08:05:55AM -0500, Rich Felker wrote: > On Wed, Feb 12, 2020 at 12:50:19PM +0100, Florian Weimer wrote: > > * Paul Eggert: > > > > > On 1/22/20 2:05 PM, Rich Felker wrote: > > >> I think we're approaching a consensus that glibc should fix this too, > > >> so then it would just be gnulib matching the fix. > > > > > > I installed the attached patch to Gnulib in preparation for the upcoming > > > glibc fix. The patch causes fchmodat with AT_SYMLINK_NOFOLLOW to work on > > > non-symlinks, and similarly for lchmod on non-symlinks. The idea is to > > > avoid this sort of problem in the future, and to let Coreutils etc. work > > > on older platforms as if glibc 2.32 (or whatever) is already in place. > > > > The lchmod implementation based on /proc tickles an XFS bug: > > > > <https://sourceware.org/ml/libc-alpha/2020-02/msg00467.html> > > Uhg, why does Linux even let the fs driver see whether the chmod is > being performed via a filename, O_PATH fd, or magic symlink in /proc? > It should just be an operation on the inode. OK, I don't think it's actually clear from the test that the use of the magic symlink is the cause. It's plausible that XFS just always returns failure on success for this operation, and I don't have XFS to test with. Note that in any case, musl's lchmod/fchmodat is not affected since it always refuses to change symlink modes; I did this because I was worried that chmod on the magic symlink in /proc might pass through not just to the symlink it refers to, but to the symlink target if one exists. With current kernel versions it seems that does not happen; is it safe to assume it doesn't? Further, I've found some inconsistent behavior with ext4: chmod on the magic symlink fails with EOPNOTSUPP as in Florian's test, but fchmod on the O_PATH fd succeeds and changes the symlink mode. This is with 5.4. Cany anyone else confirm this? Is it a problem? Rich
