Den ons 24 juli 2024 kl 15:02 skrev Daniel Sahlberg <
daniel.l.sahlb...@gmail.com>:
> Den ons 24 juli 2024 kl 14:06 skrev Andreas Stieger <
> andreas.stie...@gmx.de>:
>
>>
>> On 2024-07-24 13:35, zongganli(李宗淦) wrote:
>>
>> We found a bug in svnlook history --limit.
>>
>>
>> Can you describe how this bug manifests itself?
>>
>> It can not deal with the illegal input like "-1", as "apr_size_t" could
>> be unsigned.
>>
>>
>> Not having reviewed this in detail, but maybe we should make this similar
>> to
>>
>>
>> https://svn.apache.org/viewvc/subversion/trunk/subversion/svn/svn.c?view=markup#l2309
>>
>
> I've reviewed this and I agree that the behaviour is not correct..
>
> The problem is that the -l option should give an error message if a <= 0
> argument is given.
>
> Code:
> https://svn.apache.org/viewvc/subversion/trunk/subversion/svnlook/svnlook.c?view=markup#l2583
>
> But apr_size_t can be unsigned (on my system it is size_t). strtol() will
> happily parse "-1" but when stored in an apr_size_t it will of course be
> interpreted as a positive number. Thus the error message isn't shown.
>
> Example with Subversion 1.14.3:
> [[[
> dsg@daniel-23:~$ svnlook history -l -1 /home/dsg/repo
> REVISION PATH
> -------- ----
> 11 /
> 10 /
> 9 /
> [...]
> 1 /
> 0 /
> ]]]
>
> Example with trunk + patch:
> [[[
> dsg@daniel-23:~/svn_trunk/subversion/svnlook$ ./svnlook history -l -1
> /home/dsg/repo
> subversion/svnlook/svnlook.c:2595: (apr_err=SVN_ERR_INCORRECT_PARAMS)
> svnlook: E200004: Argument to --limit must be positive
> ]]]
>
> Thanks for the example in svn.c, I'll test it and see if it suffers the
> same problem. I agree that the code in svnlook should be the same as
> everywhere else so I'll probably change it first and then revisit the limit
> issue.
>
svn.c doesn't suffer the same problem as svnlook. This is because
svn_cl__opt_state_t.limit is an int while svnlook.c uses apr_size_t. This
of course means the limit in the svn commands can be maximum 2 billion
(which I suppose isn't a problem for most repositories).
I'm favouring changing svnlook to mimic the behaviour of svn log (et al.).
Strictly speaking this is non-backwards compatible but only on the command
line and only if someone tries to to ask for more than 2147483647 entries.
See the attached patch.
Kind regards,
Daniel
Index: subversion/svnlook/svnlook.c
===================================================================
--- subversion/svnlook/svnlook.c (revision 1919492)
+++ subversion/svnlook/svnlook.c (working copy)
@@ -363,7 +363,7 @@
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 @@
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 @@
{
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 @@
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)
