Re: svn commit: r1919535 - /subversion/trunk/subversion/svnlook/svnlook.c

On Fri, Jul 26, 2024 at 6:35 PM <[email protected]> wrote:
>
> Author: dsahlberg
> Date: Fri Jul 26 16:33:52 2024
> New Revision: 1919535
>
> URL: http://svn.apache.org/viewvc?rev=1919535&view=rev
> Log:
> Fix the check for limit < 0 in svnlook.
>
> strtol will parse a signed long and thus for example accept -1. But the limit
> field in opt_state is apr_size_t which can be an unsigned type. If this is the
> case, the later check for opt_state.limit <= 0 will not see -1 but rather
> INT_MAX (or LONG_MAX or whatever) and thus the error message "Argument to
> --limit must be positive" is never shown.
>
> Changing the check to be the same as in subversion/svn/svn.c. This has a few
> implications:
> - The limit in svn.c is int. If apr_size_t was larger this means a lower
>   maximum for --limit is now enforced.
> - svnlook and svn now behave the same with regards to the limit option.
>
> * subversion/svnlook/svnlook.c:
>   (struct svnlook_opt_state),
>   (struct svnlook_ctxt_t),
>   (struct print_history_baton): change limit from apr_size to int
>   (sub_main): Change handling of opt_id == 'l' to the same as in svn.c
>
> See dev@: https://lists.apache.org/thread/kg36nodcgtgkcfww8qy88kyl7h46ry7x
>
> Found by: [email protected]
>
>
> Modified:
>     subversion/trunk/subversion/svnlook/svnlook.c
>
> Modified: subversion/trunk/subversion/svnlook/svnlook.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnlook/svnlook.c?rev=1919535&r1=1919534&r2=1919535&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svnlook/svnlook.c (original)
> +++ subversion/trunk/subversion/svnlook/svnlook.c Fri Jul 26 16:33:52 2024
> @@ -363,7 +363,7 @@ struct svnlook_opt_state
>    const char *txn;
>    svn_boolean_t version;          /* --version */
>    svn_boolean_t show_ids;         /* --show-ids */
> -  apr_size_t limit;               /* --limit */
> +  int limit;                      /* --limit */
>    svn_boolean_t help;             /* --help */
>    svn_boolean_t no_diff_deleted;  /* --no-diff-deleted */
>    svn_boolean_t no_diff_added;    /* --no-diff-added */
> @@ -391,7 +391,7 @@ typedef struct svnlook_ctxt_t
>    svn_fs_t *fs;
>    svn_boolean_t is_revision;
>    svn_boolean_t show_ids;
> -  apr_size_t limit;
> +  int limit;
>    svn_boolean_t no_diff_deleted;
>    svn_boolean_t no_diff_added;
>    svn_boolean_t diff_copy_from;
> @@ -1579,7 +1579,7 @@ struct print_history_baton
>  {
>    svn_fs_t *fs;
>    svn_boolean_t show_ids;    /* whether to show node IDs */
> -  apr_size_t limit;          /* max number of history items */
> +  int limit;                 /* max number of history items */
>    apr_size_t count;          /* number of history items processed */
>  };
>
> @@ -2582,11 +2582,12 @@ sub_main(int *exit_code, int argc, const
>
>          case 'l':
>            {
> -            char *end;
> -            opt_state.limit = strtol(opt_arg, &end, 10);
> -            if (end == opt_arg || *end != '\0')
> +            const char *utf8_opt_arg;
> +            SVN_ERR(svn_utf_cstring_to_utf8(&utf8_opt_arg, opt_arg, pool));
> +            svn_error_t *err = svn_cstring_atoi(&opt_state.limit, utf8_opt_arg);
> +            if (err)
>                {
> -                return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> +                return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, err ,
>                                          _("Non-numeric limit argument given"));
>                }
>              if (opt_state.limit <= 0)
>
>

I found that it fails on Linux, if maintainer mode is enabled (if
configured with --enable-maintainer-mode option). Providing the
detailed log above:

[[[
subversion/svnlook/svnlook.c: In function 'sub_main':
subversion/svnlook/svnlook.c:2587:26: warning: declaration of 'err'
shadows a previous local [-Wshadow]
 2587 |             svn_error_t *err =
svn_cstring_atoi(&opt_state.limit, utf8_opt_arg);
      |                          ^~~
subversion/svnlook/svnlook.c:2471:16: note: shadowed declaration is here
 2471 |   svn_error_t *err;
      |                ^~~
subversion/svnlook/svnlook.c:2587:13: error: ISO C90 forbids mixed
declarations and code [-Werror=declaration-after-statement]
 2587 |             svn_error_t *err =
svn_cstring_atoi(&opt_state.limit, utf8_opt_arg);
      |             ^~~~~~~~~~~
]]]


--
Timofei Zhakov