pgsql: Perl scripts: eliminate "Useless interpolation" warnings

[Bruce Momjian]

Perl scripts:  eliminate "Useless interpolation" warnings

Eliminate warnings of Perl Critic from src/tools.

Backpatch-through: master

Branch
--
master

Details
---
https://git.postgresql.org/pg/commitdiff/43ce181059d4ecbb1b14b75e7f38a7dda9f80225

Modified Files
--
src/tools/copyright.pl   |  2 +-
src/tools/gen_export.pl  | 10 +-
src/tools/gen_keywordlist.pl |  4 ++--
src/tools/msvc_gendef.pl | 10 +-
src/tools/version_stamp.pl   | 18 +-
src/tools/win32tzlist.pl |  2 +-
6 files changed, 23 insertions(+), 23 deletions(-)

pgsql: Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

[Tom Lane]

Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

In existing releases of libxml2, xmlXPathCompile can be driven
to stack overflow because it fails to protect itself against
too-deeply-nested input.  While there is an upstream fix as of
yesterday, it will take years for that to propagate into all
shipping versions.  In the meantime, we can protect our own
usages basically for free by calling xmlXPathCtxtCompile instead.

(The actual bug is that libxml2 keeps its nesting counter in the
xmlXPathContext, and its parsing code was willing to just skip
counting nesting levels if it didn't have a context.  So if we supply
a context, all is well.  It seems odd actually that it works at all
to not supply a context, because this means that XPath parsing does
not have access to XML namespace info.  Apparently libxml2 never
checks namespaces until runtime?  Anyway, this seems like good
future-proofing even if its only immediate effect is to dodge a bug.)

Sadly, this hack only offers protection with libxml2 2.9.11 and newer.
Before that there are multiple similar problems, so if you are
processing untrusted XML it behooves you to get a newer version.
But we have some pretty old libxml2 in the buildfarm, so it seems
impractical to add a regression test to verify this fix.

Per bug #18617 from Jingzhou Fu.  Back-patch to all supported
versions.

Discussion: https://postgr.es/m/18617-1cee4d2ed1f4e...@postgresql.org
Discussion: https://gitlab.gnome.org/GNOME/libxml2/-/issues/799

Branch
--
master

Details
---
https://git.postgresql.org/pg/commitdiff/d5622acb32b3c11a27b323138fbee9c715742b38

Modified Files
--
contrib/xml2/xpath.c|  4 ++--
src/backend/utils/adt/xml.c | 18 +++---
2 files changed, 17 insertions(+), 5 deletions(-)

pgsql: Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

[Tom Lane]

Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

In existing releases of libxml2, xmlXPathCompile can be driven
to stack overflow because it fails to protect itself against
too-deeply-nested input.  While there is an upstream fix as of
yesterday, it will take years for that to propagate into all
shipping versions.  In the meantime, we can protect our own
usages basically for free by calling xmlXPathCtxtCompile instead.

(The actual bug is that libxml2 keeps its nesting counter in the
xmlXPathContext, and its parsing code was willing to just skip
counting nesting levels if it didn't have a context.  So if we supply
a context, all is well.  It seems odd actually that it works at all
to not supply a context, because this means that XPath parsing does
not have access to XML namespace info.  Apparently libxml2 never
checks namespaces until runtime?  Anyway, this seems like good
future-proofing even if its only immediate effect is to dodge a bug.)

Sadly, this hack only offers protection with libxml2 2.9.11 and newer.
Before that there are multiple similar problems, so if you are
processing untrusted XML it behooves you to get a newer version.
But we have some pretty old libxml2 in the buildfarm, so it seems
impractical to add a regression test to verify this fix.

Per bug #18617 from Jingzhou Fu.  Back-patch to all supported
versions.

Discussion: https://postgr.es/m/18617-1cee4d2ed1f4e...@postgresql.org
Discussion: https://gitlab.gnome.org/GNOME/libxml2/-/issues/799

Branch
--
REL_15_STABLE

Details
---
https://git.postgresql.org/pg/commitdiff/634804885f334c4125134147a05a074e600598aa

Modified Files
--
contrib/xml2/xpath.c|  4 ++--
src/backend/utils/adt/xml.c | 18 +++---
2 files changed, 17 insertions(+), 5 deletions(-)

pgsql: Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

[Tom Lane]

Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

In existing releases of libxml2, xmlXPathCompile can be driven
to stack overflow because it fails to protect itself against
too-deeply-nested input.  While there is an upstream fix as of
yesterday, it will take years for that to propagate into all
shipping versions.  In the meantime, we can protect our own
usages basically for free by calling xmlXPathCtxtCompile instead.

(The actual bug is that libxml2 keeps its nesting counter in the
xmlXPathContext, and its parsing code was willing to just skip
counting nesting levels if it didn't have a context.  So if we supply
a context, all is well.  It seems odd actually that it works at all
to not supply a context, because this means that XPath parsing does
not have access to XML namespace info.  Apparently libxml2 never
checks namespaces until runtime?  Anyway, this seems like good
future-proofing even if its only immediate effect is to dodge a bug.)

Sadly, this hack only offers protection with libxml2 2.9.11 and newer.
Before that there are multiple similar problems, so if you are
processing untrusted XML it behooves you to get a newer version.
But we have some pretty old libxml2 in the buildfarm, so it seems
impractical to add a regression test to verify this fix.

Per bug #18617 from Jingzhou Fu.  Back-patch to all supported
versions.

Discussion: https://postgr.es/m/18617-1cee4d2ed1f4e...@postgresql.org
Discussion: https://gitlab.gnome.org/GNOME/libxml2/-/issues/799

Branch
--
REL_13_STABLE

Details
---
https://git.postgresql.org/pg/commitdiff/4310dfa254e9de9b5fbecbc54ef512187a9e1d09

Modified Files
--
contrib/xml2/xpath.c|  4 ++--
src/backend/utils/adt/xml.c | 18 +++---
2 files changed, 17 insertions(+), 5 deletions(-)

pgsql: Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

[Tom Lane]

Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

In existing releases of libxml2, xmlXPathCompile can be driven
to stack overflow because it fails to protect itself against
too-deeply-nested input.  While there is an upstream fix as of
yesterday, it will take years for that to propagate into all
shipping versions.  In the meantime, we can protect our own
usages basically for free by calling xmlXPathCtxtCompile instead.

(The actual bug is that libxml2 keeps its nesting counter in the
xmlXPathContext, and its parsing code was willing to just skip
counting nesting levels if it didn't have a context.  So if we supply
a context, all is well.  It seems odd actually that it works at all
to not supply a context, because this means that XPath parsing does
not have access to XML namespace info.  Apparently libxml2 never
checks namespaces until runtime?  Anyway, this seems like good
future-proofing even if its only immediate effect is to dodge a bug.)

Sadly, this hack only offers protection with libxml2 2.9.11 and newer.
Before that there are multiple similar problems, so if you are
processing untrusted XML it behooves you to get a newer version.
But we have some pretty old libxml2 in the buildfarm, so it seems
impractical to add a regression test to verify this fix.

Per bug #18617 from Jingzhou Fu.  Back-patch to all supported
versions.

Discussion: https://postgr.es/m/18617-1cee4d2ed1f4e...@postgresql.org
Discussion: https://gitlab.gnome.org/GNOME/libxml2/-/issues/799

Branch
--
REL_17_STABLE

Details
---
https://git.postgresql.org/pg/commitdiff/b9645dca10995abb54a9e12bee6c9db579229c24

Modified Files
--
contrib/xml2/xpath.c|  4 ++--
src/backend/utils/adt/xml.c | 18 +++---
2 files changed, 17 insertions(+), 5 deletions(-)

pgsql: Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

[Tom Lane]

Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

In existing releases of libxml2, xmlXPathCompile can be driven
to stack overflow because it fails to protect itself against
too-deeply-nested input.  While there is an upstream fix as of
yesterday, it will take years for that to propagate into all
shipping versions.  In the meantime, we can protect our own
usages basically for free by calling xmlXPathCtxtCompile instead.

(The actual bug is that libxml2 keeps its nesting counter in the
xmlXPathContext, and its parsing code was willing to just skip
counting nesting levels if it didn't have a context.  So if we supply
a context, all is well.  It seems odd actually that it works at all
to not supply a context, because this means that XPath parsing does
not have access to XML namespace info.  Apparently libxml2 never
checks namespaces until runtime?  Anyway, this seems like good
future-proofing even if its only immediate effect is to dodge a bug.)

Sadly, this hack only offers protection with libxml2 2.9.11 and newer.
Before that there are multiple similar problems, so if you are
processing untrusted XML it behooves you to get a newer version.
But we have some pretty old libxml2 in the buildfarm, so it seems
impractical to add a regression test to verify this fix.

Per bug #18617 from Jingzhou Fu.  Back-patch to all supported
versions.

Discussion: https://postgr.es/m/18617-1cee4d2ed1f4e...@postgresql.org
Discussion: https://gitlab.gnome.org/GNOME/libxml2/-/issues/799

Branch
--
REL_14_STABLE

Details
---
https://git.postgresql.org/pg/commitdiff/7721fff06cc52a1208d628310ce40c3698baa2cb

Modified Files
--
contrib/xml2/xpath.c|  4 ++--
src/backend/utils/adt/xml.c | 18 +++---
2 files changed, 17 insertions(+), 5 deletions(-)

pgsql: Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

[Tom Lane]

Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

In existing releases of libxml2, xmlXPathCompile can be driven
to stack overflow because it fails to protect itself against
too-deeply-nested input.  While there is an upstream fix as of
yesterday, it will take years for that to propagate into all
shipping versions.  In the meantime, we can protect our own
usages basically for free by calling xmlXPathCtxtCompile instead.

(The actual bug is that libxml2 keeps its nesting counter in the
xmlXPathContext, and its parsing code was willing to just skip
counting nesting levels if it didn't have a context.  So if we supply
a context, all is well.  It seems odd actually that it works at all
to not supply a context, because this means that XPath parsing does
not have access to XML namespace info.  Apparently libxml2 never
checks namespaces until runtime?  Anyway, this seems like good
future-proofing even if its only immediate effect is to dodge a bug.)

Sadly, this hack only offers protection with libxml2 2.9.11 and newer.
Before that there are multiple similar problems, so if you are
processing untrusted XML it behooves you to get a newer version.
But we have some pretty old libxml2 in the buildfarm, so it seems
impractical to add a regression test to verify this fix.

Per bug #18617 from Jingzhou Fu.  Back-patch to all supported
versions.

Discussion: https://postgr.es/m/18617-1cee4d2ed1f4e...@postgresql.org
Discussion: https://gitlab.gnome.org/GNOME/libxml2/-/issues/799

Branch
--
REL_16_STABLE

Details
---
https://git.postgresql.org/pg/commitdiff/4c9bf947a9e8fc6f7f531530656c5b99a93c

Modified Files
--
contrib/xml2/xpath.c|  4 ++--
src/backend/utils/adt/xml.c | 18 +++---
2 files changed, 17 insertions(+), 5 deletions(-)

pgsql: Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

[Tom Lane]

Replace usages of xmlXPathCompile() with xmlXPathCtxtCompile().

In existing releases of libxml2, xmlXPathCompile can be driven
to stack overflow because it fails to protect itself against
too-deeply-nested input.  While there is an upstream fix as of
yesterday, it will take years for that to propagate into all
shipping versions.  In the meantime, we can protect our own
usages basically for free by calling xmlXPathCtxtCompile instead.

(The actual bug is that libxml2 keeps its nesting counter in the
xmlXPathContext, and its parsing code was willing to just skip
counting nesting levels if it didn't have a context.  So if we supply
a context, all is well.  It seems odd actually that it works at all
to not supply a context, because this means that XPath parsing does
not have access to XML namespace info.  Apparently libxml2 never
checks namespaces until runtime?  Anyway, this seems like good
future-proofing even if its only immediate effect is to dodge a bug.)

Sadly, this hack only offers protection with libxml2 2.9.11 and newer.
Before that there are multiple similar problems, so if you are
processing untrusted XML it behooves you to get a newer version.
But we have some pretty old libxml2 in the buildfarm, so it seems
impractical to add a regression test to verify this fix.

Per bug #18617 from Jingzhou Fu.  Back-patch to all supported
versions.

Discussion: https://postgr.es/m/18617-1cee4d2ed1f4e...@postgresql.org
Discussion: https://gitlab.gnome.org/GNOME/libxml2/-/issues/799

Branch
--
REL_12_STABLE

Details
---
https://git.postgresql.org/pg/commitdiff/0206795d2595e60117dbb4729e2f0ba904b39bd6

Modified Files
--
contrib/xml2/xpath.c|  4 ++--
src/backend/utils/adt/xml.c | 18 +++---
2 files changed, 17 insertions(+), 5 deletions(-)

Re: pgsql: Perl scripts: eliminate "Useless interpolation" warnings

[Andrew Dunstan]

On 2024-09-15 Su 10:56 AM, Bruce Momjian wrote:

Perl scripts:  eliminate "Useless interpolation" warnings

Eliminate warnings of Perl Critic from src/tools.

Backpatch-through: master




I don't understand this commit. The buildfarm members crake and koel 
regularly run the perl critic checks and have not complained. See for 
example from before this change: 



The change doesn't seem to have had any discussion either. This 
particular warning is surely a very low level (i.e. fairly unimportant) 
one, of the type we normally ignore. If some version of perlcritic has 
raised it to severity 5 then the correct action IMNSHO would be to add 
an exception for it to the perlcriticrc, like we do for 
ProhibitLeadingZeros. If not, then perhaps you can explain how you got 
the warnings.



cheers


andrew



--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: pgsql: Perl scripts: eliminate "Useless interpolation" warnings

[Bruce Momjian]

On Sun, Sep 15, 2024 at 02:30:47PM -0400, Andrew Dunstan wrote:
> 
> 
> On 2024-09-15 Su 10:56 AM, Bruce Momjian wrote:
> 
> Perl scripts:  eliminate "Useless interpolation" warnings
> 
> Eliminate warnings of Perl Critic from src/tools.
> 
> Backpatch-through: master
> 
> 
> 
> I don't understand this commit. The buildfarm members crake and koel regularly
> run the perl critic checks and have not complained. See for example from 
> before
> this change:  koel&dt=2024-09-15%2003%3A34%3A02&stg=perl-check>
> 
> The change doesn't seem to have had any discussion either. This particular
> warning is surely a very low level (i.e. fairly unimportant) one, of the type
> we normally ignore. If some version of perlcritic has raised it to severity 5
> then the correct action IMNSHO would be to add an exception for it to the
> perlcriticrc, like we do for ProhibitLeadingZeros. If not, then perhaps you 
> can
> explain how you got the warnings.

So, the warning is about the use of double-quotes when single-quotes
will work just fine.  I wrote a new script and changed it to single
quotes, so for consistency, I looked at other Perl scripts that might
have the issue.  The message I got was:

   /root/add_commit_links.pl: Useless interpolation of literal string at line 
51 near 'my $tmpfile = $file . ".tmp";'.  (Severity: 1)

My $HOME/.perlcritic has:

severity = 1

and that is why I saw it.  Is it a bad change?

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"

Re: pgsql: Perl scripts: eliminate "Useless interpolation" warnings

[Andrew Dunstan]

On 2024-09-15 Su 4:16 PM, Bruce Momjian wrote:

On Sun, Sep 15, 2024 at 02:30:47PM -0400, Andrew Dunstan wrote:


On 2024-09-15 Su 10:56 AM, Bruce Momjian wrote:

 Perl scripts:  eliminate "Useless interpolation" warnings

 Eliminate warnings of Perl Critic from src/tools.

 Backpatch-through: master



I don't understand this commit. The buildfarm members crake and koel regularly
run the perl critic checks and have not complained. See for example from before
this change:


The change doesn't seem to have had any discussion either. This particular
warning is surely a very low level (i.e. fairly unimportant) one, of the type
we normally ignore. If some version of perlcritic has raised it to severity 5
then the correct action IMNSHO would be to add an exception for it to the
perlcriticrc, like we do for ProhibitLeadingZeros. If not, then perhaps you can
explain how you got the warnings.

So, the warning is about the use of double-quotes when single-quotes
will work just fine.  I wrote a new script and changed it to single
quotes, so for consistency, I looked at other Perl scripts that might
have the issue.  The message I got was:

/root/add_commit_links.pl: Useless interpolation of literal string at line 51 near 
'my $tmpfile = $file . ".tmp";'.  (Severity: 1)

My $HOME/.perlcritic has:

severity = 1

and that is why I saw it.  Is it a bad change?



I understand perfectly what the warning is about.

But the project's perlcritic policy is expressed at 
src/tools/perlcheck/perlcriticrc. It's basically severity 5 plus some 
additions and one exception. We shouldn't be imposing our personal 
perlcritic policies on the project.


The change isn't bad in itself, but there shouldn't be any expectation 
that we will keep to this policy, as it's not required by project policy.


There is a huge mass of perlcritic issues in our perl code at severity 1 
(over 13000 by my count). If we're going to clean them up (which I would 
oppose) we should do it in a systematic way. It's hard to see why this 
one is special.


cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: pgsql: Perl scripts: eliminate "Useless interpolation" warnings

[Bruce Momjian]

On Sun, Sep 15, 2024 at 04:33:49PM -0400, Andrew Dunstan wrote:
> I understand perfectly what the warning is about.
> 
> But the project's perlcritic policy is expressed at src/tools/perlcheck/
> perlcriticrc. It's basically severity 5 plus some additions and one exception.
> We shouldn't be imposing our personal perlcritic policies on the project.
> 
> The change isn't bad in itself, but there shouldn't be any expectation that we
> will keep to this policy, as it's not required by project policy.
> 
> There is a huge mass of perlcritic issues in our perl code at severity 1 (over
> 13000 by my count). If we're going to clean them up (which I would oppose) we
> should do it in a systematic way. It's hard to see why this one is special.

I guess it is special only because my policy is more strict so I wanted
my new code to match.  Should I revert it?  Is the very tiny improvement
not worth the churn in the code?

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"

Re: pgsql: Perl scripts: eliminate "Useless interpolation" warnings

[Andrew Dunstan]

On 2024-09-15 Su 4:36 PM, Bruce Momjian wrote:

On Sun, Sep 15, 2024 at 04:33:49PM -0400, Andrew Dunstan wrote:

I understand perfectly what the warning is about.

But the project's perlcritic policy is expressed at src/tools/perlcheck/
perlcriticrc. It's basically severity 5 plus some additions and one exception.
We shouldn't be imposing our personal perlcritic policies on the project.

The change isn't bad in itself, but there shouldn't be any expectation that we
will keep to this policy, as it's not required by project policy.

There is a huge mass of perlcritic issues in our perl code at severity 1 (over
13000 by my count). If we're going to clean them up (which I would oppose) we
should do it in a systematic way. It's hard to see why this one is special.

I guess it is special only because my policy is more strict so I wanted
my new code to match.  Should I revert it?



Yes I think so.



Is the very tiny improvement
not worth the churn in the code?



Yeah, I don't think it is.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

pgsql: Perl scripts: revert 43ce181059d

[Bruce Momjian]

Perl scripts:  revert 43ce181059d

Small improvement not worth the code churn.

Reported-by: Andrew Dunstan

Discussion: 
https://postgr.es/m/42f2242a-422b-4aa3-8d60-d67b229c4...@dunslane.net

Backpatch-through: master

Branch
--
master

Details
---
https://git.postgresql.org/pg/commitdiff/4632e5cf4bc5c496f41dfc6a89533e7afa7262dd

Modified Files
--
src/tools/copyright.pl   |  2 +-
src/tools/gen_export.pl  | 10 +-
src/tools/gen_keywordlist.pl |  4 ++--
src/tools/msvc_gendef.pl | 10 +-
src/tools/version_stamp.pl   | 18 +-
src/tools/win32tzlist.pl |  2 +-
6 files changed, 23 insertions(+), 23 deletions(-)

Re: pgsql: Perl scripts: eliminate "Useless interpolation" warnings

[Bruce Momjian]

On Sun, Sep 15, 2024 at 06:07:21PM -0400, Andrew Dunstan wrote:
> 
> On 2024-09-15 Su 4:36 PM, Bruce Momjian wrote:
> > On Sun, Sep 15, 2024 at 04:33:49PM -0400, Andrew Dunstan wrote:
> > > I understand perfectly what the warning is about.
> > > 
> > > But the project's perlcritic policy is expressed at src/tools/perlcheck/
> > > perlcriticrc. It's basically severity 5 plus some additions and one 
> > > exception.
> > > We shouldn't be imposing our personal perlcritic policies on the project.
> > > 
> > > The change isn't bad in itself, but there shouldn't be any expectation 
> > > that we
> > > will keep to this policy, as it's not required by project policy.
> > > 
> > > There is a huge mass of perlcritic issues in our perl code at severity 1 
> > > (over
> > > 13000 by my count). If we're going to clean them up (which I would 
> > > oppose) we
> > > should do it in a systematic way. It's hard to see why this one is 
> > > special.
> > I guess it is special only because my policy is more strict so I wanted
> > my new code to match.  Should I revert it?
> 
> 
> Yes I think so.

Okay, done.

> > Is the very tiny improvement
> > not worth the churn in the code?
> 
> 
> Yeah, I don't think it is.

FYI, the line that got me started was:

my ($tmpfilename) = $filename . ".tmp";

while I would write that with single quotes.  Because mine uses single
quotes and version_stamp.pl uses double-quotes, I started to try to
unify the style.

I agree severity=1 generates many false warnings, and changing code
based on them can actually produce errors, but I do think there are some
safe ones like the single/double quote item we can improve.

Attached is my Perl critic file, where I do use severity=1, but turn off
various warnings.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"
# syntax 
https://github.com/schwern/Perl-Critic/blob/master/examples/perlcriticrc

verbose = 7
severity = 1
# list from --profile-proto
# --verbose 8 outputs critic labels

# Write open $handle, $path instead of open($handle, $path).
[-CodeLayout::ProhibitParensWithBuiltins]

# Must run code through perltidy.
[-CodeLayout::RequireTidyCode]

# Write if($condition){ do_something() } instead of do_something() if 
$condition.
[-ControlStructures::ProhibitPostfixControls]

# Give every module a $VERSION number.
[-Modules::RequireVersionVar]

# Write @{ $array_ref } instead of @$array_ref.
[-References::ProhibitDoubleSigils]

# Always use the /s modifier with regular expressions.
[-RegularExpressions::RequireDotMatchAnything]

# Always use the /x modifier with regular expressions.
[-RegularExpressions::RequireExtendedFormatting]

# Always use the /m modifier with regular expressions.
[-RegularExpressions::RequireLineBoundaryMatching]

# Use character classes for literal meta-characters instead of escapes.
[-RegularExpressions::ProhibitEscapedMetacharacters]

# Don't write sub my_function (@@) {}.
[-Subroutines::ProhibitSubroutinePrototypes]

# Write q{} instead of ''.
[-ValuesAndExpressions::ProhibitEmptyQuotes]

# Don't use values that don't explain themselves.
[-ValuesAndExpressions::ProhibitMagicNumbers]