On Mon, May 20, 2024 at 7:03 AM Nathan Hartman <hartman.nat...@gmail.com> wrote:
>
> On Sun, May 19, 2024 at 4:09 PM Timofey Zhakov <t...@chemodax.net> wrote:
> >
> > Hi,
> >
> > I found a little bug in parsing a change revision: If the number,
> > given to the --change argument, starts with a double minus or with
> > `-r-`, the command aborts. This patch fixes this bug.
> >
> > Steps to reproduce:
> >
> > $ svn diff https://svn.apache.org/repos/asf -c --123
> > svn: E235000: In file '..\..\..\subversion\libsvn_client\ra.c' line
> > 692: assertion failed (SVN_IS_VALID_REVNUM(start_revnum))
> >
> > Or...
> >
> > $ svn diff https://svn.apache.org/repos/asf -c -r-123
> > svn: E235000: In file '..\..\..\subversion\libsvn_client\ra.c' line
> > 692: assertion failed (SVN_IS_VALID_REVNUM(start_revnum))
> >
> > The same would happen if the svn diff command is invoked from a
> > working copy, without URL.
> >
> > [[[
> > Fix bug: check the change argument for a double minus at the start.
> >
> > If changeno is negative and is_negative is TRUE, raise
> > SVN_ERR_CL_ARG_PARSING_ERROR, because string with a double minus is
> > not a valid number.
> >
> > * subversion/svn/svn.c
> > (sub_main): If changeno is negative and is_negative is TRUE, raise
> > SVN_ERR_CL_ARG_PARSING_ERROR.
> > * subversion/tests/cmdline/diff_tests.py
> > (diff_invalid_change_arg): New test.
> > (test_list): Run new test.
> > ]]]
> >
> > Best regards,
> > Timofei Zhakov
>
>
> Good catch! I agree we should issue a proper error and not assert.
>
> While studying this change, I noticed a second similar bug if we are
> given a range and the second value is negative.
>
> In other words, this works correctly:
>
> $ svn diff -c r1917671-1917672
> http://svn.apache.org/repos/asf/subversion/trunk
>
> But this doesn't; notice the double minus signs:
>
> $ svn diff -c r1917671--1917672
> http://svn.apache.org/repos/asf/subversion/trunk
> svn: E235000: In file 'subversion/libsvn_client/ra.c' line 682:
> assertion failed (SVN_IS_VALID_REVNUM(start_revnum))
> Abort trap: 6
>
> The first minus sign is correctly interpreted to indicate a range, but
> the second minus sign is read by strtol(). That's several lines above
> your change in svn.c (line 2379 on trunk@1917826). We are doing:
>
> changeno_end = strtol(s, &end, 10);
>
> but we are not checking if 'changeno_end' is negative (which it isn't
> allowed to be).
>
> I'm okay to commit this patch now and handle the second bug in a
> follow-up, or handle both bugs together. Let me know what you prefer.
>
> Cheers,
> Nathan
Thank you for your response!
I didn't notice the second bug. I think it would be better to commit
them together, so I fixed and added it to the new patch. The patch is
in the attachments.
Here is an updated log message:
[[[
Fix bug: check the change argument for a double minus.
If the changeno is negative and is_negative is TRUE, raise
SVN_ERR_CL_ARG_PARSING_ERROR, because string with a double minus is
not a valid number. Do the same if the changeno_end is negative.
* subversion/svn/svn.c
(sub_main): If the changeno is negative and is_negative is TRUE, raise
SVN_ERR_CL_ARG_PARSING_ERROR. Do the same if the changeno_end is
negative.
* subversion/tests/cmdline/diff_tests.py
(diff_invalid_change_arg): New test.
(test_list): Run new test.
]]]
Additionally, I am thinking about moving the code, which parses a
change revision, into a function in the opt.c file, like
svn_opt_parse_revision_to_range.
Thanks!
Index: subversion/svn/svn.c
===================================================================
--- subversion/svn/svn.c (revision 1917823)
+++ subversion/svn/svn.c (working copy)
@@ -2377,6 +2377,15 @@ sub_main(int *exit_code, int argc, const char *arg
while (*s == 'r')
s++;
changeno_end = strtol(s, &end, 10);
+
+ if (changeno_end < 0)
+ {
+ return svn_error_createf(
+ SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("Negative number in range (%s)"
+ " not supported with -c"),
+ change_str);
+ }
}
if (end == change_str || *end != '\0')
{
@@ -2391,6 +2400,14 @@ sub_main(int *exit_code, int argc, const char *arg
_("There is no change 0"));
}
+ /* The revision number cannot contain a double minus */
+ if (changeno < 0 && is_negative)
+ {
+ return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("Non-numeric change argument "
+ "(%s) given to -c"), change_str);
+ }
+
if (is_negative)
changeno = -changeno;
Index: subversion/tests/cmdline/diff_tests.py
===================================================================
--- subversion/tests/cmdline/diff_tests.py (revision 1917823)
+++ subversion/tests/cmdline/diff_tests.py (working copy)
@@ -5329,7 +5329,37 @@ def diff_nonexistent_in_wc(sbox):
svntest.actions.run_and_verify_svn(expected_output_head_base, [],
'diff', '-r', '1')
+def diff_invalid_change_arg(sbox):
+ "invalid change argument"
+ sbox.build()
+
+ svntest.actions.run_and_verify_svn(
+ None,
+ (r'.*svn: E205000: Non-numeric change argument \(--1\) given to -c'),
+ 'diff', sbox.wc_dir, '-c', '--1')
+
+ svntest.actions.run_and_verify_svn(
+ None,
+ (r'.*svn: E205000: Non-numeric change argument \(-r-1\) given to -c'),
+ 'diff', sbox.wc_dir, '-c', '-r-1')
+
+ svntest.actions.run_and_verify_svn(
+ None,
+ (r'.*svn: E205000: Negative number in range \(1--3\) not supported with
-c'),
+ 'diff', sbox.wc_dir, '-c', '1--3')
+
+ # 'r' is not a number
+ svntest.actions.run_and_verify_svn(
+ None,
+ (r'.*svn: E205000: Non-numeric change argument \(r1--r3\) given to -c'),
+ 'diff', sbox.wc_dir, '-c', 'r1--r3')
+
+ svntest.actions.run_and_verify_svn(
+ None,
+ (r'.*svn: E205000: Negative number in range \(r1-r-3\) not supported with
-c'),
+ 'diff', sbox.wc_dir, '-c', 'r1-r-3')
+
########################################################################
#Run the tests
@@ -5431,6 +5461,7 @@ test_list = [ None,
diff_file_replaced_by_symlink,
diff_git_format_copy,
diff_nonexistent_in_wc,
+ diff_invalid_change_arg,
]
if __name__ == '__main__':
