On Fri, Jul 26, 2024 at 6:35 PM <dsahlb...@apache.org> 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: zongga...@tencent.com
>
>
> 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
