297928 - detaint_natural and trick_taint shouldn't rely on $1.

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Christian Reis]

detaint_natural and trick_taint currently rely on $1 being defined or not after
a match. However, Perl doesn't reset $1 when a match fails! This causes very
hard-to-debug failures inside our detainting functions. 

The workaround is to use the return value of the match expression.

sub detaint_natural {
  ($ret) = $_[0] =~ /^(\d+)$/;
  $_[0] = $ret;
  return (defined($_[0]));
}

This may have hidden some errors deep down in Bugzilla's regexp-land..

Comment 1

[Christian Reis]

Attachment: kiko_v1: use the return value of the match instead of $1

If you are observant you will notice there is still a $1 used in
detaint_signed. I think that one's safe because we test the return value of the
match before using it.

Comment 2

[Joel Peshkin]

How about....

sub detaint_natural {
  if ($_[0] =~ /^(\d+)$/) {
      return $_[0];
  } else {
      return undef;
  }
}

??

Comment 3

[Dave Miller [:justdave]]

which branch does that apply to, and do we have backports for the others yet?

Comment 4

[Christian Reis]

That was against head -- I produced no branch patches.

Comment 5

[Frédéric Buclin]

(In reply to comment #3)
> which branch does that apply to, and do we have backports for the others yet?

This patch applies cleanly to 2.18. 2.16 does not have detaint_signed.

Comment 6

[Frédéric Buclin]

Attachment: 2.16 backport, v1

Comment 7

[Frédéric Buclin]

Tip:

Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.28; previous revision: 1.27
done


2.18.1:

Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.12.2.4; previous revision: 1.12.2.3
done


2.16.10:

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.169.2.30; previous revision: 1.169.2.29
done

Comment 8

[Max Kanat-Alexander]

What about this would you like in the relnotes?

Comment 9

[Max Kanat-Alexander]

I can't figure out what to relnote about this. If you'd like to have it
relnoted, please re-add the keyword and explain what should go in the relnotes.