[PATCH] Trim dead code in do_kv

[p5pRT]

Migrated from rt.perl.org#77290 (status was 'resolved')

Searchable as RT77290$

[p5pRT]

From @ikegami

Trim dead code in do_kv.

A small piece of code in do_kv has three bugs​:

- TARG could have been returned as an lvalue\, so its refcount   could be greater than 1\, resulting in data getting clobbered.   (See RT#20933 for previously fixed occurrence of this bug).

- LvTARG is refcounted\, so it's buggy to just NULL it.

- TARG is returned without being initialised.

The first two bugs disappeared recently when we stopped putting the lvalues in TARG for that op. The third remains.

However\, it seems that code is never called. It can only be called by putting NULL (not undef) on the Perl stack. I don't see how that's possible here. The test suite never reaches that code\, so it seems it's just dead code.

- Eric Brine

[p5pRT]

From @ikegami

0001-Trim-dead-code-in-do_kv.patch

```diff From f9e7c075b2eca7616794d893f1cb1dac122ea93b Mon Sep 17 00:00:00 2001 From: Eric Brine Date: Tue, 17 Aug 2010 19:50:40 -0700 Subject: [PATCH] Trim dead code in do_kv. A small piece of code in do_kv has three bugs: - TARG could have been returned as an lvalue, so its refcount could be greater than 1, resulting in data getting clobbered. (See RT#20933 for previously fixed occurrence of this bug). - LvTARG is refcounted, so it's buggy to just NULL it. - TARG is returned without being initialised. The first two bugs disappeared recently when we stopped putting the lvalues in TARG for that op. The third remains. However, it seems that code is never called. It can only be called by putting NULL (not undef) on the Perl stack. I don't see how that's possible here. The test suite never reaches that code, so it seems it's just dead code. --- doop.c | 16 ++-------------- 1 files changed, 2 insertions(+), 14 deletions(-) diff --git a/doop.c b/doop.c index 903144c..8c142da 100644 --- a/doop.c +++ b/doop.c @@ -1431,25 +1431,13 @@ Perl_do_kv(pTHX) { dVAR; dSP; - HV * const hv = MUTABLE_HV(POPs); - HV *keys; + HV * const keys = MUTABLE_HV(POPs); register HE *entry; const I32 gimme = GIMME_V; const I32 dokv = (PL_op->op_type == OP_RV2HV || PL_op->op_type == OP_PADHV); const I32 dokeys = dokv || (PL_op->op_type == OP_KEYS); const I32 dovalues = dokv || (PL_op->op_type == OP_VALUES); - if (!hv) { - if (PL_op->op_flags & OPf_MOD || LVRET) { /* lvalue */ - dTARGET; /* make sure to clear its target here */ - if (SvTYPE(TARG) == SVt_PVLV) - LvTARG(TARG) = NULL; - PUSHs(TARG); - } - RETURN; - } - - keys = hv; (void)hv_iterinit(keys); /* always reset iterator regardless */ if (gimme == G_VOID) @@ -1491,7 +1479,7 @@ Perl_do_kv(pTHX) if (dovalues) { SV *tmpstr; PUTBACK; - tmpstr = hv_iterval(hv,entry); + tmpstr = hv_iterval(keys,entry); DEBUG_H(Perl_sv_setpvf(aTHX_ tmpstr, "%lu%%%d=%lu", (unsigned long)HeHASH(entry), (int)HvMAX(keys)+1, -- 1.7.1.1 ```

[p5pRT]

From @nwc10

On Tue\, Aug 17\, 2010 at 08​:15​:35PM -0700\, Eric Brine wrote​:

# New Ticket Created by "Eric Brine" # Please include the string​: [perl #77290] # in the subject line of all future correspondence about this issue. # \<URL​: http​://rt.perl.org/rt3/Ticket/Display.html?id=77290 >

Trim dead code in do_kv.

A small piece of code in do_kv has three bugs​:

- TARG could have been returned as an lvalue\, so its refcount could be greater than 1\, resulting in data getting clobbered. (See RT#20933 for previously fixed occurrence of this bug).

- LvTARG is refcounted\, so it's buggy to just NULL it.

- TARG is returned without being initialised.

The first two bugs disappeared recently when we stopped putting the lvalues in TARG for that op. The third remains.

Do you happen to (easily) know the commit that changed lvalues into TARG?

However\, it seems that code is never called. It can only be called by putting NULL (not undef) on the Perl stack. I don't see how that's possible here. The test suite never reaches that code\, so it seems it's just dead code.

I *had* assumed that it was not valid to put NULL on the Perl stack. However\, it can happen for invalid glob lookups - the GV argument popped by pp_readdir in this code is NULL​:

$ ./perl -Ilib -MO=Concise -e '$x = "."; readdir($x)' a \<@​> leave[1 ref] vKP/REFC ->(end) 1 \<0> enter ->2 2 \<;> nextstate(main 1 -e​:1) v​:{ ->3 5 \<2> sassign vKS/2 ->6 3 \<$> const(PV ".") s ->4 - \<1> ex-rv2sv sKRM*/1 ->5 4 \<$> gvsv(*x) s ->5 6 \<;> nextstate(main 1 -e​:1) v​:{ ->7 9 \<1> readdir vK/1 ->a 8 \<1> rv2gv sK*/1 ->9 - \<1> ex-rv2sv sK/1 ->8 7 \<$> gvsv(*x) s ->8 -e syntax OK

$ ./perl -Ds -e '$x = "."; readdir($x)'

EXECUTING...

  =>
  =>
  =>
  => PV("."\0)
  => PV("."\0) UNDEF
  => PV("."\0)
  =>
  => PV("."\0)
  => VOID
Bad symbol for dirhandle at -e line 1.

That's NULL for a GV. I don't know if there's any way to get NULL on the stack for an HV.

Nicholas Clark

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @ikegami

On Wed\, Aug 18\, 2010 at 5​:34 AM\, Nicholas Clark \nick@&#8203;ccl4\.org wrote​:

The first two bugs disappeared recently when we stopped putting the lvalues in TARG for that op. The third remains.

Do you happen to (easily) know the commit that changed lvalues into TARG?

Into TARG? I think lvalue keys always used TARG. http​://perl5.git.perl.org/perl.git/commitdiff/85581909df34d9ffca6c85cafeb2595c4cb89ffb

Away from TARG? RT#67838 http​://perl5.git.perl.org/perl.git/commit/e2fe06dd0f4d62a54d7bbc2a1f42dae0dd6bf19e http​://perl5.git.perl.org/perl.git/commit/213f7ada530c1a4c4148622ba78577585f8ab9d0 http​://perl5.git.perl.org/perl.git/commit/0607bed5b8805b56401c29fc8c6d0f3737d5353f http​://perl5.git.perl.org/perl.git/commit/2154eca77956ce145743765bea9ce269e6227984

However\, it seems that code is never called. It can only be called by putting NULL (not undef) on the Perl stack. I don't see how that's possible here. The test suite never reaches that code\, so it seems it's just dead code.

I *had* assumed that it was not valid to put NULL on the Perl stack. However\, it can happen for invalid glob lookups - the GV argument popped by pp_readdir in this code is NULL​:

Should that be changed?

- Eric Brine

[p5pRT]

From @cpansprout

On Wed Aug 18 09​:02​:31 2010\, ikegami@​adaelis.com wrote​:

On Wed\, Aug 18\, 2010 at 5​:34 AM\, Nicholas Clark \nick@&#8203;ccl4\.org wrote​:

The first two bugs disappeared recently when we stopped putting the lvalues in TARG for that op. The third remains.

Do you happen to (easily) know the commit that changed lvalues into TARG?

Into TARG? I think lvalue keys always used TARG.

http​://perl5.git.perl.org/perl.git/commitdiff/85581909df34d9ffca6c85cafeb2595c4cb89ffb

Away from TARG? RT#67838

http​://perl5.git.perl.org/perl.git/commit/e2fe06dd0f4d62a54d7bbc2a1f42dae0dd6bf19e

http​://perl5.git.perl.org/perl.git/commit/213f7ada530c1a4c4148622ba78577585f8ab9d0

http​://perl5.git.perl.org/perl.git/commit/0607bed5b8805b56401c29fc8c6d0f3737d5353f

http​://perl5.git.perl.org/perl.git/commit/2154eca77956ce145743765bea9ce269e6227984

However\, it seems that code is never called. It can only be called by putting NULL (not undef) on the Perl stack. I don't see how that's possible here. The test suite never reaches that code\, so it seems it's just dead code.

I *had* assumed that it was not valid to put NULL on the Perl stack. However\, it can happen for invalid glob lookups - the GV argument popped by pp_readdir in this code is NULL​:

As I noted elsewhere\, it happened by accident in commit 7a5fd60d4c.

Should that be changed?

It has been\, with commit 99fc7eca40.

So I think your patch is safe to apply\, and it has been applied as 73ff03e.

(I’m actually going to make some changes that will put nulls on the stack on purpose\, but they will not affect keys and values.)

[p5pRT]

@cpansprout - Status changed from 'open' to 'resolved'