On Wed, May 5, 2021 at 11:03 AM Ryan Long <ryan.l...@oarcorp.com> wrote: > > Reply is below. > > -----Original Message----- > From: Gedare Bloom <ged...@rtems.org> > Sent: Tuesday, May 4, 2021 11:27 AM > To: Ryan Long <ryan.l...@oarcorp.com> > Cc: Sebastian Huber <sebastian.hu...@embedded-brains.de>; devel@rtems.org > Subject: Re: [PATCH v1 1/5] libcsupport: Added futimens() and utimensat() > > On Tue, May 4, 2021 at 10:04 AM Ryan Long <ryan.l...@oarcorp.com> wrote: > > > > > > > > -----Original Message----- > > From: Sebastian Huber <sebastian.hu...@embedded-brains.de> > > Sent: Tuesday, May 4, 2021 12:04 AM > > To: Ryan Long <ryan.l...@oarcorp.com>; devel@rtems.org > > Subject: Re: [PATCH v1 1/5] libcsupport: Added futimens() and > > utimensat() > > > > On 03/05/2021 15:40, Ryan Long wrote: > > > > > Created futimens.c and utimensat.c to add support for the POSIX > > > methods futimens() and utimensat(). > > > > > > utime() and utimes() are considered obsolote by POSIX, but RTEMS > > > will continue to support them. > > > > > > Closes #4396 > > > --- > > > cpukit/Makefile.am | 2 + > > > cpukit/include/rtems/libio_.h | 61 +++++++++- > > > cpukit/libcsupport/src/futimens.c | 91 +++++++++++++++ > > > cpukit/libcsupport/src/utimensat.c | 229 > > > +++++++++++++++++++++++++++++++++++++ > > > spec/build/cpukit/librtemscpu.yml | 2 + > > > 5 files changed, 384 insertions(+), 1 deletion(-) > > > create mode 100644 cpukit/libcsupport/src/futimens.c > > > create mode 100644 cpukit/libcsupport/src/utimensat.c > > > > > > diff --git a/cpukit/Makefile.am b/cpukit/Makefile.am index > > > b0df610..29b4207 100644 > > > --- a/cpukit/Makefile.am > > > +++ b/cpukit/Makefile.am > > > @@ -262,6 +262,8 @@ librtemscpu_a_SOURCES += libcsupport/src/unmount.c > > > librtemscpu_a_SOURCES += libcsupport/src/__usrenv.c > > > librtemscpu_a_SOURCES += libcsupport/src/utime.c > > > librtemscpu_a_SOURCES += libcsupport/src/utimes.c > > > +librtemscpu_a_SOURCES += libcsupport/src/futimens.c > > > +librtemscpu_a_SOURCES += libcsupport/src/utimensat.c > > > librtemscpu_a_SOURCES += libcsupport/src/utsname.c > > > librtemscpu_a_SOURCES += libcsupport/src/vprintk.c > > > librtemscpu_a_SOURCES += libcsupport/src/write.c diff --git > > > a/cpukit/include/rtems/libio_.h b/cpukit/include/rtems/libio_.h > > > index > > > e9eb462..fc7a1e4 100644 > > > --- a/cpukit/include/rtems/libio_.h > > > +++ b/cpukit/include/rtems/libio_.h > > > @@ -7,7 +7,7 @@ > > > */ > > > > > > /* > > > - * COPYRIGHT (c) 1989-2011. > > > + * COPYRIGHT (c) 1989-2011, 2021. > > > * On-Line Applications Research Corporation (OAR). > > > * > > > * Modifications to support reference counting in the file system > > > are @@ -357,6 +357,65 @@ static inline void > > > rtems_filesystem_instance_unlock( > > > (*mt_entry->ops->unlock_h)( mt_entry ); > > > } > > > > > > +/* Prototypes for functions used between utimensat() and futimens() > > > +*/ > > > + > > > +/** > > > + * @brief Checks the tv_sec member of a timespec struct > > > + * > > > + * @param[in] time The timespec struct to be validated > > > + * > > > + * Ensures that the value in the tv_sec member is non-negative. > > > + * > > > + * @retval true If the tv_sec member is a valid value > > > + * @retval false If the tv_sec member is not a valid value */ bool > > > +rtems_filesystem_utime_tv_sec_valid( struct timespec time ); > > > + > > > +/** > > > + * @brief Checks the tv_nsec member of a timespec struct > > > + * > > > + * Ensures that the value in the tv_nsec member is equal to either > > > + * UTIME_NOW, UTIME_OMIT, or a value greater-than or equal to zero > > > + * and less than a billion. > > > + * > > > + * @param[in] time The timespec struct to be validated > > > + * > > > + * @retval true If the tv_nsec member is a valid value > > > + * @retval false If the tv_nsec member is not a valid value */ > > > +bool rtems_filesystem_utime_tv_nsec_valid( struct timespec time ); > > > > Are these two functions ever used individually? Do they modify the time? > > Maybe we should add a standard phase for boolean type returns: > > > > @return Returns true, if the X is valid, otherwise false. > > > > [Ryan Long] These functions are only used in > > rtems_filesystem_utime_check_times(). They don't modify the time, they just > > ensure that the members of the timespec struct are valid values. Should the > > arguments for those functions be changed to be a const since they're not > > being modified? And sure, I can consolidate the @retval statements like > > that. > > > > > + > > > +/** > > > + * @brief Determines if the process has write permissions to a file > > > + * > > > + * Checks that the process has the same userID as the file and > > > +whether the > > > + * file has write permissions. > > > + * > > > + * @param[in] currentloc The current location to a file > > > + * @param[in] fstat_h The file handler of @currentloc > > Why is this an extra parameter? > > [Ryan Long] This was Joel's suggestion, but I didn't think about just using > > the currentloc argument. I'll change that in V2. > > > + * > > > + * @retval 0 If the process has write permissions > > > + * @retval -1 If the process does not have write permissions > > Why 0 and -1, what about errno? Is it set by the function or not? > > [Ryan Long] It's 0 and -1 because it uses the > > rtems_set_errno_and_return_minus_one() macro. > > > + */ > > > +int rtems_filesystem_utime_check_permissions( > > > + const rtems_filesystem_location_info_t *currentloc, > > > + rtems_filesystem_fstat_t fstat_h > > > +); > > > + > > > +/** > > > + * @brief Checks @times and updates @new_times > > > + * > > > + * @param[in] times The timespec struct to be checked > > > + * @param[in,out] new_times The timespec struct to contain the > > > +updated time > > > + * > > > + * @retval 0 If the time was able to be updated > > > + * @retval -1 If the time was not able to be updated */ int > > > +rtems_filesystem_utime_check_times( > > > + const struct timespec times[2], > > > + struct timespec new_times[2] > > > +); > > > + > > > /* > > > * File Descriptor Routine Prototypes > > > */ > > > diff --git a/cpukit/libcsupport/src/futimens.c > > > b/cpukit/libcsupport/src/futimens.c > > > new file mode 100644 > > > index 0000000..33ae469 > > > --- /dev/null > > > +++ b/cpukit/libcsupport/src/futimens.c > > > @@ -0,0 +1,91 @@ > > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > > + > > > +/** > > > + * @file > > > + * > > > + * @ingroup libcsupport > > > + * > > > + * @brief Set file access and modification times based on file > > > +descriptor in > > > + * nanoseconds. > > > + */ > > > + > > > +/* > > > + * COPYRIGHT (c) 1989-2009, 2021. > > > + * On-Line Applications Research Corporation (OAR). > > > + * > > > + * Redistribution and use in source and binary forms, with or > > > +without > > > + * modification, are permitted provided that the following > > > +conditions > > > + * are met: > > > + * 1. Redistributions of source code must retain the above copyright > > > + * notice, this list of conditions and the following disclaimer. > > > + * 2. Redistributions in binary form must reproduce the above copyright > > > + * notice, this list of conditions and the following disclaimer in the > > > + * documentation and/or other materials provided with the > > > distribution. > > > + * > > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > > > "AS IS" > > > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > > > +LIMITED TO, THE > > > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A > > > +PARTICULAR PURPOSE > > > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR > > > +CONTRIBUTORS BE > > > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, > > > +OR > > > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, > > > +PROCUREMENT OF > > > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > > > +BUSINESS > > > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, > > > +WHETHER IN > > > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR > > > +OTHERWISE) > > > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF > > > +ADVISED OF THE > > > + * POSSIBILITY OF SUCH DAMAGE. > > > + */ > > > + > > > +#ifdef HAVE_CONFIG_H > > > +#include "config.h" > > > +#endif > > > + > > > +#include <sys/stat.h> > > > +#include <rtems/libio_.h> > > > + > > > +#include <fcntl.h> > > > +#include <stdlib.h> > > > +#include <string.h> > > > + > > > +/** > > > + * > > > +https://pubs.opengroup.org/onlinepubs/9699919799.2008edition/functi > > > +on > > > +s/futimens.html > > > + * > > > + * Set file access and modification times */ int futimens( > > > + int fd, > > > + const struct timespec times[2] > > > +) > > > +{ > > > + int rv; > > > + rtems_libio_t *iop; > > > + struct timespec new_times[2]; > > > + const rtems_filesystem_location_info_t *currentloc = NULL; > > > + > > > + LIBIO_GET_IOP_WITH_ACCESS( fd, iop, LIBIO_FLAGS_READ, EBADF ); > > > + > > > + currentloc = &iop->pathinfo; > > > + > > > + rv = rtems_filesystem_utime_check_permissions( > > > + currentloc, > > > + (*currentloc->handlers->fstat_h) ); if( rv == -1 ) { > > > > Which coding style should this file and the other new files have? > > > > [...] > > > > > +} > > > + > > > +/* Determine whether the access and modified timestamps can be > > > +updated */ int rtems_filesystem_utime_check_permissions( > > > + const rtems_filesystem_location_info_t * currentloc, > > > + rtems_filesystem_fstat_t fstat_h > > > +) > > > +{ > > > + struct stat st; > > > + volatile int rv; > > volatile? > > [Ryan Long] Forgot to take this out after debugging. Will be removed in V2. > > > + bool write_access; > > > + > > > + memset( &st, 0, sizeof(st) ); > > > + > > > + rv = (*fstat_h)( currentloc, &st ); if( rv != 0 ) { > > > + rtems_set_errno_and_return_minus_one( ENOENT ); } > > > + > > > + /* > > > + * Check that the user ID and the user ID of the file match > > > + */ > > > + if( geteuid() != st.st_uid ) { > > > + rtems_set_errno_and_return_minus_one( EACCES ); } > > Is this really what POSIX specified? > > [Ryan Long] Yes, on > > https://pubs.opengroup.org/onlinepubs/9699919799.2008edition/functions/futimens.html > > they said return this if 'the effective user ID of the process does not > > match the owner of the file and write access is denied.' > > > + > > But you haven't checked write access yet. I think you haven't parsed the > specification correctly. > [Ryan Long] So I should move the assignment of write_access above this check, > and change the check to be this? > yep. Other cases might be missing or conflicting though, so you'll need to check through all the logic, e.g., EPERM.
> if( geteuid() != st.st_uid) { > if( !write_access) { > rtems_set_errno_and_return_minus_one( EACCES ); > } > } > > > > + write_access = rtems_filesystem_check_access( > > > + RTEMS_FS_PERMS_WRITE, > > > + st.st_mode, > > > + st.st_uid, > > > + st.st_gid > > > + ); > > > + if( !write_access ) { > > > + rtems_set_errno_and_return_minus_one( EACCES ); } > > > + > > > + return 0; > > > +} > > > > > [...] > > > + > > > + > > > +/** > > > + * > > > +https://pubs.opengroup.org/onlinepubs/9699919799.2008edition/functi > > > +ons/futimens.html > > > + * > > > + * Set file access and modification times */ int utimensat( > > > + int fd, > > > + const char *path, > > > + const struct timespec times[2], > > > + int flag > > > +) > > > +{ > > > + int rv = 0; > > > + rtems_filesystem_eval_path_context_t ctx; > > > + int eval_flags = RTEMS_FS_FOLLOW_LINK; > > > + const rtems_filesystem_location_info_t *currentloc = NULL; > > > + struct timespec new_times[2]; > > > + > > > + /* > > > + * RTEMS does not currently support operating on a real file descriptor > > > + */ > > > + if( fd != AT_FDCWD ) { > > > + rtems_set_errno_and_return_minus_one( ENOSYS ); } > > > + > > > + /* > > > + * RTEMS does not currently support AT_SYMLINK_NOFOLLOW > > > + */ > > > + if( flag != 0 ) { > > > + rtems_set_errno_and_return_minus_one( ENOSYS ); } > > > + > > > + currentloc = rtems_filesystem_eval_path_start( &ctx, path, > > > + eval_flags ); > > > + > > > + rv = rtems_filesystem_utime_check_permissions( > > > + currentloc, > > > + (*currentloc->handlers->fstat_h) ); if( rv == -1 ) { > > Better use the success code, for example rv != 0. > > > + rtems_filesystem_eval_path_cleanup( &ctx ); > > > + return rv; > > > + } > > > + > > > + rv = rtems_filesystem_utime_check_times( times, new_times); if( > > > + rv == -1 ) { > > > + rtems_filesystem_eval_path_cleanup( &ctx ); > > > + return rv; > > > + } > > > + > > > + rv = (*currentloc->mt_entry->ops->utimens_h)( > > > + currentloc, > > > + new_times > > > + ); > > > + > > > + rtems_filesystem_eval_path_cleanup( &ctx ); > > > + > > > + return rv; > > > +} > > > diff --git a/spec/build/cpukit/librtemscpu.yml > > > b/spec/build/cpukit/librtemscpu.yml > > > index a3a9ee4..9639051 100644 > > > --- a/spec/build/cpukit/librtemscpu.yml > > > +++ b/spec/build/cpukit/librtemscpu.yml > > > @@ -761,6 +761,8 @@ source: > > > - cpukit/libcsupport/src/unmount.c > > > - cpukit/libcsupport/src/utime.c > > > - cpukit/libcsupport/src/utimes.c > > > +- cpukit/libcsupport/src/futimens.c > > > +- cpukit/libcsupport/src/utimensat.c > > > - cpukit/libcsupport/src/utsname.c > > > - cpukit/libcsupport/src/vprintk.c > > > - cpukit/libcsupport/src/write.c > > > > -- > > embedded brains GmbH > > Herr Sebastian HUBER > > Dornierstr. 4 > > 82178 Puchheim > > Germany > > email: sebastian.hu...@embedded-brains.de > > phone: +49-89-18 94 741 - 16 > > fax: +49-89-18 94 741 - 08 > > > > Registergericht: Amtsgericht München > > Registernummer: HRB 157899 > > Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas > > Dörfler Unsere Datenschutzerklärung finden Sie hier: > > https://embedded-brains.de/datenschutzerklaerung/ > > > > _______________________________________________ > > devel mailing list > > devel@rtems.org > > http://lists.rtems.org/mailman/listinfo/devel _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel