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
