Bug#890810: Handling of symbolic links has been broken

2018-04-26 Thread Jonathan de Boyne Pollard
(My apologies for the late reply.  Your mail never arrived, and I only 
discovered it on the WWW interface yesterday.)


Thorsten Glaser:

I’ve noticed an error (in the native BSD environment) which I believe 
to be related, if not the same, but haven’t gotten around to debugging 
yet.


I’ll have to find a way to do this on systems without the |*at()| 
syscalls, however. Since you already looked into this, perhaps you 
have an idea for me ;)


I have.  I've tested this on Debian Linux.  It highlights that there's a 
further bug somewhere else, with the datestamps that are being supplied 
for symbolic links to these functions; but it addresses /this/ problem 
of symbolic links being erroneously followed.


The FreeBSD |pax| code in comparison uses |lutimes()|, but using 
|utimensat()| eliminates the need for an extra |lstat()| call because of 
the availablility of the |UTIME_OMIT| mechanism.  Both OpenBSD and 
FreeBSD have a |utimensat()| function, and neither of their |pax| 
implementations have this problem with following symbolic links. I'll 
leave you with what if anything to do about MirOS BSD.  (-:


In |file_subs.c| add


#if defined(__LINUX__) || defined(__linux__) || defined(__FreeBSD__) || defined 
(__DragonFlyBSD__) || defined(__OpenBSD__)
#define PAX_UTIMENSAT
#define PAX_FUTIMENS
#elif !defined(__INTERIX)
#define PAX_UTIMES
#endif

And rewrite these two functions:

void
set_ftime(char *fnm, time_t mtime, time_t atime, int frc)
{
#if defined(PAX_UTIMENSAT)
 static struct timespec ts[2] = {{0L, 0L}, {0L, 0L}};
#elif defined(PAX_UTIMES)
 static struct timeval tv[2] = {{0L, 0L}, {0L, 0L}};
 struct stat sb;
#else
 struct utimbuf u;
 struct stat sb;
#endif

 if (!frc && (!patime || !pmtime)) {
 /*
  * If we are not forcing, only set those times the user wants set.
  * We get the current values of the times if we need them.
  */
#if defined(PAX_UTIMENSAT)
 if (!patime)
 ts[0].tv_nsec = UTIME_OMIT;
 if (!pmtime)
 ts[1].tv_nsec = UTIME_OMIT;
#else
 if (lstat(fnm, ) == 0) {
 if (!patime)
 atime = sb.st_atime;
 if (!pmtime)
 mtime = sb.st_mtime;
 } else
 syswarn(0,errno,"Unable to obtain file stats %s", fnm);
#endif
 }

 /*
  * set the times
  */
#if defined(PAX_UTIMENSAT)
 ts[0].tv_sec = atime;
 ts[1].tv_sec = mtime;
 if (utimensat(AT_FDCWD, fnm, ts, AT_SYMLINK_NOFOLLOW) < 0)
#elif defined(PAX_UTIMES)
 tv[0].tv_sec = (long)atime;
 tv[1].tv_sec = (long)mtime;
 if (utimes(fnm, tv) < 0)
#else
 u.actime = atime;
 u.modtime = mtime;
 if (utime(fnm, ) < 0)
#endif
 syswarn(1, errno, "Access/modification time set failed on: %s",
 fnm);
}

#if defined(PAX_FUTIMES)
static void
fset_ftime(char *fnm, int fd, time_t mtime, time_t atime, int frc)
{
#if defined(PAX_FUTIMENS)
 static struct timespec ts[2] = {{0L, 0L}, {0L, 0L}};
#else
 static struct timeval tv[2] = {{0L, 0L}, {0L, 0L}};
 struct stat sb;
#endif

 if (!frc && (!patime || !pmtime)) {
 /*
  * If we are not forcing, only set those times the user wants set.
  * We get the current values of the times if we need them.
  */
#if defined(PAX_FUTIMENS)
 if (!patime)
 ts[0].tv_nsec = UTIME_OMIT;
 if (!pmtime)
 ts[1].tv_nsec = UTIME_OMIT;
#else
 if (fstat(fd, ) == 0) {
 if (!patime)
 atime = sb.st_atime;
 if (!pmtime)
 mtime = sb.st_mtime;
 } else
 syswarn(0,errno,"Unable to obtain file stats %s", fnm);
#endif
 }
 /*
  * set the times
  */
#if defined(PAX_FUTIMENS)
 ts[0].tv_sec = atime;
 ts[1].tv_sec = mtime;
 if (futimens(fd, ts) < 0)
#else
 tv[0].tv_sec = (long)atime;
 tv[1].tv_sec = (long)mtime;
 if (futimes(fd, tv) < 0)
#endif
 syswarn(1, errno, "Access/modification time set failed on: %s",
 fnm);
}
#endif


Here's an alternative rendition that makes the individual 
implementations clearer, but makes the code prone to maintenance errors 
where only one of the alternatives gets updated:



void
set_ftime(char *fnm, time_t mtime, time_t atime, int frc)
{
#if defined(PAX_UTIMENSAT)
 static struct timespec ts[2] = {{0L, 0L}, {0L, 0L}};

 if (!frc && (!patime || !pmtime)) {
 /*
  * If we are not forcing, only set those times the user wants set.
  */
 if (!patime)
 ts[0].tv_nsec = UTIME_OMIT;
 if (!pmtime)
 ts[1].tv_nsec = UTIME_OMIT;
 }

 /*
  * set the times
  */
 ts[0].tv_sec = atime;
 ts[1].tv_sec = mtime;
 if (utimensat(AT_FDCWD, fnm, ts, AT_SYMLINK_NOFOLLOW) < 0)
 syswarn(1, errno, "Access/modification time set failed on: %s",
 fnm);
#elif defined(PAX_UTIMES)
 

Bug#890810: Handling of symbolic links has been broken

2018-02-19 Thread Thorsten Glaser
Hi Jonathan,

> pax is incorrectly using utimes() to attempt to set the timestamps of symbolic
> links, rather than utimensat() with AT_SYMLINK_NOFOLLOW.

thanks for the preliminary analysis. I’ve noticed an error (in the
native BSD environment) which I believe to be related, if not the
same, but haven’t gotten around to debugging yet.

I’ll have to find a way to do this on systems without the *at()
syscalls, however. Since you already looked into this, perhaps
you have an idea for me ;)

Thanks,
//mirabilos
-- 
“It is inappropriate to require that a time represented as
 seconds since the Epoch precisely represent the number of
 seconds between the referenced time and the Epoch.”
-- IEEE Std 1003.1b-1993 (POSIX) Section B.2.2.2



Bug#890810: Handling of symbolic links has been broken

2018-02-18 Thread Jonathan de Boyne Pollard

Package: pax
Version: 1:20161104-2

pax is incorrectly using utimes() to attempt to set the timestamps of 
symbolic links, rather than utimensat() with AT_SYMLINK_NOFOLLOW.


$ install -d -m 0755 a b
$ ln -s /etc/motd a/c
$ cd a
$ pax -r -w * ../b/
pax: Access/modification time set failed on: ../b/c: Operation not permitted
$ strace -e utimes pax -r -w * ../b/
utimes("../b/c", [{tv_sec=1519020077, tv_usec=0}, {tv_sec=1519020077, 
tv_usec=0}]) = -1 EPERM (Operation not permitted)

pax: Access/modification time set failed on: ../b/c: Operation not permitted
+++ exited with 1 +++
$

Run this as an unprivileged user.  As a privileged user, pax ends up 
quietly stomping over the timestamps of whatever is being symbolically 
linked to, which can have all sorts of unintended side-effects depending 
from what files end up having their timestamps adjusted.


This is a regression from version 1:20140703-2 .

$ pax -r -w * ../b/
$ strace -e utimes pax -r -w * ../b/
+++ exited with 0 +++
$