Re: [PATCH v2] cvsserver: avoid precedence problem between ! and %s

[Ondrej Pohorelsky <opohorel@redhat.com>]

On Wed, May 21, 2025 at 7:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> >> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>
> >>
> >> With perl-5.41.4 and newer, git-cvsserver fails to build because of
> >> possible precedence problem[0]
> >
> > What is the exact symptom?  As Perl is not a language to compile and
> > run separately, "fails to build" does not look like what exactly is
> > going on.  "gives a warning and then refuses to run"?  "gives a warning
> > before running"?  Something else?
>
> Stepping back a bit, the original that the new warning complains
> about is this:
>
> -    if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
>
> And the complaint is that due to operator binding precedence, this
> does
>
>     (!$refname) =~ /pattern/
>
> Unless the behaviour has changed as well as warning, which is highly
> unlikely, doesn't it mean that the code was wrong, with or without
> the warning, all along?  The intent of the code was to see if the
> refname conformed to dotted decimal, and if it does not, the refname
> gets munged in the block guarded by that if (condition).  But the
> condition was a total nonsense.  !$refname would most likely to be
> an empty string (unless $refname contains '0' or an empty string),
> which would not match the /^[1-9][0-9]*(\.[1-9][0-9]*)*$/ pattern,
> so we probably have always munged the $refName in escapeRefName sub.
>
> Which I do not see used anywhere in the program, though.  There is a
> call to unescapeRefname method, but it seems to me that
> escapeRefName is never called.
>
> What made you send a patch for this program?  Do you or anybody you
> know use git-cvsserver?  Unless I am reading the program
> incorrectly, despite the claim in front of that escapeRefName sub
> that we avoid sending a tag whose name is not something CVS would be
> happy with, we did not sanitize the refs and relied solely on the
> users' repository to use only safe characters in the refs to keep
> CVS clients happy, and the fact that this expression used as if()
> condition is totally broken does not really make any difference,
> since it is in an unused sub.  I have to wonder if (1) it is a
> better fix to just remove the unused sub, and/or (2) perhaps nobody
> uses cvsserver to allow cvs clients to talk to a Git repository?
>


What I meant by 'does not build' is that the warnings that were added
in the newest Perl release populate the cvs.log when running the test
suite.
This causes some tests from t9402-git-cvsserver-refs.sh to fail, which
then fails the whole build in Fedora.
Tests that are affected are t9402.30, t9402.31, t9402.32, t9402.34.


> >> Added parentheses avoid this issue.
> >
> > We phrase such "this is how the patch addresses the issue" statement
> > in imperative, as if we are telling the codebase to become-like-so,
> > e.g., "Enclose the pattern matching =~ in parentheses to force the
> > right order of binding", or something like that.
>


I'll rephrase the commit message to meet this requirement.


On Wed, May 21, 2025 at 7:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Ondřej Pohořelský via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> >> From: =?UTF-8?q?Ond=C5=99ej=20Poho=C5=99elsk=C3=BD?= <opohorel@redhat.com>
> >>
> >> With perl-5.41.4 and newer, git-cvsserver fails to build because of
> >> possible precedence problem[0]
> >
> > What is the exact symptom?  As Perl is not a language to compile and
> > run separately, "fails to build" does not look like what exactly is
> > going on.  "gives a warning and then refuses to run"?  "gives a warning
> > before running"?  Something else?
>
> Stepping back a bit, the original that the new warning complains
> about is this:
>
> -    if(! $refName=~/^[1-9][0-9]*(\.[1-9][0-9]*)*$/)
>
> And the complaint is that due to operator binding precedence, this
> does
>
>     (!$refname) =~ /pattern/
>
> Unless the behaviour has changed as well as warning, which is highly
> unlikely, doesn't it mean that the code was wrong, with or without
> the warning, all along?  The intent of the code was to see if the
> refname conformed to dotted decimal, and if it does not, the refname
> gets munged in the block guarded by that if (condition).  But the
> condition was a total nonsense.  !$refname would most likely to be
> an empty string (unless $refname contains '0' or an empty string),
> which would not match the /^[1-9][0-9]*(\.[1-9][0-9]*)*$/ pattern,
> so we probably have always munged the $refName in escapeRefName sub.
>
> Which I do not see used anywhere in the program, though.  There is a
> call to unescapeRefname method, but it seems to me that
> escapeRefName is never called.
>
> What made you send a patch for this program?  Do you or anybody you
> know use git-cvsserver?  Unless I am reading the program
> incorrectly, despite the claim in front of that escapeRefName sub
> that we avoid sending a tag whose name is not something CVS would be
> happy with, we did not sanitize the refs and relied solely on the
> users' repository to use only safe characters in the refs to keep
> CVS clients happy, and the fact that this expression used as if()
> condition is totally broken does not really make any difference,
> since it is in an unused sub.  I have to wonder if (1) it is a
> better fix to just remove the unused sub, and/or (2) perhaps nobody
> uses cvsserver to allow cvs clients to talk to a Git repository?
>
> >> Added parentheses avoid this issue.
> >
> > We phrase such "this is how the patch addresses the issue" statement
> > in imperative, as if we are telling the codebase to become-like-so,
> > e.g., "Enclose the pattern matching =~ in parentheses to force the
> > right order of binding", or something like that.
>


-- 

Ondřej Pohořelský

Software Engineer

Red Hat

opohorel@redhat.com