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

Hey, I'm a Perl newbie heh and I was just wondering if any of the people here could give me any feed back, this is a chat program I made, (well the actual program part) it is put into a frame with a page that it appends to that is located outside of my cgi-bin so that perl scripts are'nt executed, any ways here it is lol. 
#!/usr/local/bin/perl

use CGI;

use CGI ':standard';
print header;
print start_html('Chat'),
h1('Chat'),
    start_form,
    "Name ",textfield('name'),
    p,
    "Say ",textfield('say'),
    p, 
    submit,
    end_form,
    hr;

if (param()) {
    
$say=(param('say')); 
$name=(param('name'));
       
     
    $say =~  s/</&lt;/g; #Stops user from entering the < in both name 
+and speech
    $name =~ s/</&lt;/g; #Leaves the > allowed, you can't enter HTML w
+ithout <
        
        $say =~ s/!{5}/!/g;  #Although this won't take care of all mul
+tiple ! it can reduce 
                 #the number to help avoid multiple lines of just !'s 
        
     open CHAT, ">>../chatty2.html";
        print CHAT "<h5><i>",$name,": </i><b>",$say,"</b></h5>\n";
        close CHAT

    }
      
 print end_html;

[download]
I'm proably doing a bunch of stuff wrong heh, any feed back you can provide would be helpful, thanks :)

Replies are listed 'Best First'.
Re: Seeking Feed back
by azatoth (Curate) on Jul 05, 2001 at 12:34 UTC

    For starters, all people new to Perl should be made aware of strict.pm. This is a Perl Module that ensures your program runs smoothly with regard to Scoping. You should also read up on use warnings; or the -w switch, which basically adds an extra level of integrity to your program.

    Also on your filehandle call, you should always finish the statement off with or die "Could not open $fh : $!\n"; The die function enables error handling, storing the error value in the special variable $!. So your code should now look like 

    open CHAT, ">>../chatty2.html" or die "Could not open file : $!\n";
# your script will fail if you can't open the file, and print the reas
+on why

    [download]
    In fact, you should use die on most function calls. Read up on that, it'll save you a lot of heartache in the future.

    Finally, enjoy your time on Perlmonks. It's a very nice place.

    Azatoth a.k.a Captain Whiplash

    Make Your Die Messages Full of Wisdom!
    Get YOUR PerlMonks Stagename here!
    Want to speak like a Londoner?
[reply]
[d/l]
[select]
      Actually, in my 1200-line Mapster program (currently down), I used constructs like: 
      if(open FILE, ">$filename") {
  flock FILE, 2;
  ... # Write stuff to FILE
  close FILE;
} else {
  print "An I/O error occurred!";
  log_error $!; # Call a sub
}

      [download]
      This allows my program to exit gracefully in the unlikely event of an error.
[reply]
[d/l]
        And so you have an error, you know that there is no such file or directory. What then? How does this help in debugging.

        As perlstyle says: 

               o   Always check the return codes of system calls.  Good
           error messages should go to STDERR, include which
           program caused the problem, what the failed system
           call and arguments were, and (VERY IMPORTANT) should
           contain the standard system error message for what
           went wrong.  Here's a simple but sufficient example:

               opendir(D, $dir)     or die "can't opendir $dir: $!";

        [download]
        By not including debugging information on your arguments you will make such basic things as a renamed directory painful to track down.
[reply]
[d/l]

        what?  if () {} else {} ?

        what about: 

        open FILE, ">$filename" or &exit_with_error ('An I/O error occured!', 
+$!);

        [download]
        and in your &exit_with_error sub you do more stuff and exit

        just a thought

        He who asks will be a fool for five minutes, but he who doesn't ask will remain a fool for life.

        Chady | http://chady.net/
[reply]
[d/l]
[select]
Re: Seeking Feed back
by Maestro_007 (Hermit) on Jul 05, 2001 at 19:20 UTC
    One more thing - I would add -T to the shebang line. You do a few checks on the data to make sure nobody's trying to hack it, but, to quote various sources on the subject, "never trust any input from the user".

    Your checks are good for what you've thought of, but there may be some things that you haven't thought of. Also, you may end up altering the code some day in such a way to make these security holes more apparent and exploitable.

    MM

[reply]
[d/l]
A reply falls below the community's threshold of quality. You may see it by logging in.

Back to Seekers of Perl Wisdom