Den lör 13 jan. 2024 kl 09:58 skrev <dsahlb...@apache.org>:
> Author: dsahlberg
> Date: Sat Jan 13 08:56:50 2024
> New Revision: 1915214
>
> URL: http://svn.apache.org/viewvc?rev=1915214&view=rev
> Log:
> Replace the homegrown checks for readonly/executable with calls to
> access(2)
> to consider, for example, user's secondary groups.
>
> * subversion/libsvn_subr/io.c
> (svn_io__is_finfo_read_only): As above
> (svn_io__is_finfo_executable): As above but move the permission check
> code
> from here
> (svn_io_is_file_executable): .. to here since access() wants a path and
> we
> already have it in the arguments.
>
> Suggested by: Joe Orton
> See https://lists.apache.org/list?d...@apr.apache.org
>
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/io.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/io.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/io.c?rev=1915214&r1=1915213&r2=1915214&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/io.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/io.c Sat Jan 13 08:56:50 2024
> @@ -2531,27 +2531,14 @@ svn_io__is_finfo_read_only(svn_boolean_t
> apr_pool_t *pool)
> {
> #if defined(APR_HAS_USER) && !defined(WIN32) &&!defined(__OS2__)
> - apr_status_t apr_err;
> - apr_uid_t uid;
> - apr_gid_t gid;
> -
> - *read_only = FALSE;
> -
> - apr_err = apr_uid_current(&uid, &gid, pool);
> -
> - if (apr_err)
> - return svn_error_wrap_apr(apr_err, _("Error getting UID of process"));
> -
> - /* Check write bit for current user. */
> - if (apr_uid_compare(uid, file_info->user) == APR_SUCCESS)
> - *read_only = !(file_info->protection & APR_UWRITE);
> -
> - else if (apr_gid_compare(gid, file_info->group) == APR_SUCCESS)
> - *read_only = !(file_info->protection & APR_GWRITE);
> -
> - else
> - *read_only = !(file_info->protection & APR_WWRITE);
> -
> + *read_only = (access(file_info->fname, W_OK) != 0);
> + /* svn_io__is_finfo_read_only can be called with a dangling
> + * symlink. access() will check the permission on the missing
> + * target and return -1 and errno = ENOENT. Check for ENOENT
> + * and pretend the file is writeable, otherwise we will get
> + * spurious Reverted messages on the symlink.
> + */
> + if (*read_only && errno == ENOENT) *read_only = FALSE;
> #else /* WIN32 || __OS2__ || !APR_HAS_USER */
> *read_only = (file_info->protection & APR_FREADONLY);
> #endif
> @@ -2564,33 +2551,7 @@ svn_io__is_finfo_executable(svn_boolean_
> apr_finfo_t *file_info,
> apr_pool_t *pool)
> {
> -#if defined(APR_HAS_USER) && !defined(WIN32) &&!defined(__OS2__)
>
I'm not sure if we need to keep APR_HAS_USER here. It was required for...
> - apr_status_t apr_err;
> - apr_uid_t uid;
> - apr_gid_t gid;
> -
> - *executable = FALSE;
> -
> - apr_err = apr_uid_current(&uid, &gid, pool);
>
... this function.
> -
> - if (apr_err)
> - return svn_error_wrap_apr(apr_err, _("Error getting UID of process"));
> -
> - /* Check executable bit for current user. */
> - if (apr_uid_compare(uid, file_info->user) == APR_SUCCESS)
> - *executable = (file_info->protection & APR_UEXECUTE);
> -
> - else if (apr_gid_compare(gid, file_info->group) == APR_SUCCESS)
> - *executable = (file_info->protection & APR_GEXECUTE);
> -
> - else
> - *executable = (file_info->protection & APR_WEXECUTE);
> -
> -#else /* WIN32 || __OS2__ || !APR_HAS_USER */
> - *executable = FALSE;
> -#endif
> -
> - return SVN_NO_ERROR;
> + return svn_io_is_file_executable(executable, file_info->fname, pool);
> }
>
> svn_error_t *
> @@ -2599,12 +2560,7 @@ svn_io_is_file_executable(svn_boolean_t
> apr_pool_t *pool)
> {
> #if defined(APR_HAS_USER) && !defined(WIN32) &&!defined(__OS2__)
>
Same here...
> - apr_finfo_t file_info;
> -
> - SVN_ERR(svn_io_stat(&file_info, path, APR_FINFO_PROT | APR_FINFO_OWNER,
> - pool));
> - SVN_ERR(svn_io__is_finfo_executable(executable, &file_info, pool));
> -
> + *executable = (access(path, X_OK) == 0);
> #else /* WIN32 || __OS2__ || !APR_HAS_USER */
> *executable = FALSE;
> #endif
>
>
>
Your thoughts?
Kind regards,
Daniel
