Den tors 25 juli 2024 kl 04:04 skrev zongganli(李宗淦) <zongga...@tencent.com>:
> Thank you for your reply. It does seem better to change svnlook to mimic > the behaviour of svn log. We will do this the same way in our subversion. > Thanks for the report! I have committed a fix in /trunk/ in r1919535. I'm leaning towards not backportint this change but rather let it wait for 1.15. It is a non-backwards compatible change (on platforms where apr_size_t is larger than int) but on the other hand the risk of someone using a --limit larger than 2 billion is relatively low. Kind regards, Daniel > > > Daniel Sahlberg<daniel.l.sahlb...@gmail.com> 在 2024年7月25日 周四 2:27 写道： > 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 >> >>
