Russ has asked for the wisdom of the Perl Monks concerning the following question:

I was frightened today. A co-worker showed me some code from a client. We are co-developing a site with him. He apparently does this all the time: 
foreach $key (CGI::param()) { ${$key} = CGI::param($key) }

[download]
(Assigning the HTML field name to the symbol table)

Now, he can use the HTML field names as variables (e.g. $Username, $Password, etc.) My co-worker defended this idea, saying it is entirely normal (he did it "all the time" in TCL), and that this feels much more readable to him.

So, my question: is this a "normal" thing to do (and I've just never stumbled across the idea) or am I right to have my skin crawl thinking of this?

This just can't be a "good thing," but it's not my code and not my project, so...

<cringing>Russ</cringing>

P.S. I explained the dangers of messing with the symbol table, the risks that someone will name a field (in HTML) to a used/reserved name in Perl, and the loss of maintainability (a variable which just "magically" appears without being referenced anywhere should confuse people). I told him it is considered good practice to run with 'use strict', and that this will not. In the end, I conceded that "yes, the code will work that way..." and our client wrote it, so...

Replies are listed 'Best First'.
Re: Putting HTML fieldnames in symbol space
by lhoward (Vicar) on May 27, 2000 at 02:58 UTC
    The CGI module already has a method to do what you describe above: 
      my $query=new CGI();
  $query->import_names('C');
  print "C - $C::foo";

    [download]
    For safety's sake it doesn't let you import into the main namespace. It is not safe to import into the main namespace (or other namespace that you don't reserve for just storing CGI params) because a cracker could overwrite any of your variables. Consider what would happen with the code I put below if a hacker "submitted" to the form with (the CGI formatted version of ) "cmd=rm -f /*". Where you weren't expecting cmd to be passed as a CGI param. 
    my $cmd="ls";
foreach $key (CGI::param()) { ${$key} = CGI::param($key) }
system $cmd;

    [download]
    Kind of a silly example but it does demonstrate the veunerability of arbitrarily importing CGI parameters into the main namespace.
[reply]
[d/l]
[select]
      This is a very nice example. However, you shouldn't have declared $cmd lexically--doing so means that your example isn't very dangerous at all. :)

      Because that for loop overwrites $main::cmd... not the lexical $cmd that you've already defined. When you use $cmd w/o using a package qualifier, you're using the lexical $cmd, if one exists--and one does exist, in this case. So you're still just doing 

          system "ls";

      [download]
      Make your $cmd a package global, and then it's dangerous again.

      Of course, your point is made either way. :)

[reply]
[d/l]
      Great point, lhoward!!! I was too shocked to come up with the most important reason not to do this. It is definitely a security breach waiting to happen.

      Of course, no one is 'system'ing anything from the user (I'd have fainted dead away (and flat forbidden him from doing so) had they been doing that!), but security is certainly the best argument against this nonsense.

      Thanks!

[reply]
Re: Putting HTML fieldnames in symbol space
by btrott (Parson) on May 27, 2000 at 02:52 UTC
    Well, I'd agree with you, if that helps. :)

    The person who wrote this code did something pretty silly by using symbolic references. I don't understand why people write things like this (well, okay, so I do: partly, because they come from other languages where it's encouraged or looked upon as the norm). But why do this when you could use a hash and make it *so* much easier for yourself?

    Because as silly as that was to use symbolic refs initially, it's even sillier to *defend* them. It's *not* normal. It may be used often, but it's not used by people who know of a better way.

    For what it's worth, here's some more ammunition against using symbolic refs: http://www.plover.com/~mjd/perl/varvarname2.html.

    In the end, it's not your code--and if their system is going to break because of their broken programming techniques... hopefully that won't be your problem. :)

[reply]
      Thanks for the link. That's what I was looking for from the Perl Monks community -- ammunition in information.

      Update After discussing this further, he has agreed to handle this code differently (and I'll talk to the client next week), if for no other reason than to pacify me. ;-)

      Russ

[reply]
Re: Putting HTML fieldnames in symbol space
by chromatic (Archbishop) on Jun 06, 2000 at 21:54 UTC
    What does it gain, besides a few keystrokes? If the programmer already knows the field names (if he's accessing them via $Username and $Password, it's obvious he does), you're only a few characters shorter than $q->param('Username') or $q->param('Password'). Besides being a big security hole, it doesn't really gain anything and it's less efficient. (Aliasing the variables takes time and memory.)

    I'm coming down heavily on the side of the skin-crawlers. That's, at best, a useless technique.

[reply]
Re: Putting HTML fieldnames in symbol space
by Jenda (Abbot) on Aug 11, 2006 at 01:14 UTC

    (Comming very late to the discussion via "Random Node" ...) If you do find this in some code and do not have time to switch over to $q->param("Foo") or something or if you feel too strongly that you need to save a few keystrokes, you may do this: 

    ${$_} = CGI::param($_) for qw(Username Password Email);

    [download]
    This way you know what parameters you expect and what variables will get their values from the query. No surprises with variables being overwritten by an unexpected query parameter.

    Jenda
    Support Denmark!
    Defend the free world!

[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom