Generally, your script looks fairly nice. Here are some pointers, though.

    use CGI qw(:all);
[download]

The "qw(:all)" is used by CGI.pm to export functions to your programs namespace. Since you are using the object oriented interface and not the functional one, you don't need to export those functions. Just "use CGI".

Potential security hole: most CGI scripts using CGI.pm should have the following lines before instantiation of the CGI object (or first use of a CGI function):

    $CGI::DISABLE_UPLOADS = 1;                    # Disable uploads
    $CGI::POST_MAX        = 512 * 1024;           # limit posts to 512
+K max
                                                  # set to personal ne
+eds, of course
[download]

These lines help to prevent DOS attacks based upon attackers trying to upload arbitrarily large files. If you're interested, you can use CGI::Safe which will handle that for you almost seamlessly. Just use the following lines:

    use CGI::Safe;
    my $CGI = CGI::Safe->new;
[download]

For your script, nothing else will change. The module should be considered beta, though. Let me know if there are any problems.

Also, the file you write to may occassionally get corrupted if more than one instance of your script accesses it at a time. To protect against this, create a 'semaphore' file, open it, then flock it. Then, when another instance of the script comes along, it will fail to get the lock on the semaphore, if the lock is still in place, and not cause data corruption problems on the lock file.

Cheers,
Ovid

Vote for paco!

Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.

Consider two people hitting this code at almost the same time. Since the code is being multitasked, it is possible that one script will zero out the counter right before the next one reads it... Something like:

A: read counter      Data: linux:700 windows:6
A: zero out counter  Data: --------------------
B: read counter      Data: linux:0   windows:0
A: write out counter Data: linux:701 windows:6
B: zero out counter  Data: --------------------
B: write counter     Data: linux:1   windows:0  <-- Doh!
[download]

-Blake

I would make sure you include this stuff:

use CGI;

$CGI::DISABLE_UPLOADS = 0;  # enable uploads
$CGI::POST_MAX = 1048576;   # limit the maximum upload size to 1MB
my $q = new CGI;            # OK to create new CGI object now

# make the environment safer as recommended in Perlsec
delete @ENV{qw(IFS CDPATH ENV BASH_ENV)};

# you usually have to set this to keep taint happy
$ENV{'PATH'}   = $some_paths; # typically /usr/bin:/usr/local/bin

$ENV{'TMPDIR'} = $my_temp_dir; # aim CGI.pm at a local dir
[download]

cheers

tachyon

s&&rsenoyhcatreve&&&s&n.+t&"$'$`$\"$\&"&ee&&y&srve&&d&&print

#!/usr/bin/perl -Tw
use strict;
use CGI;
use Fcntl qw(:flock);

my $CGI = new CGI;
my $file;
my $countfile="downloadcounts.txt";
my $OS_Type=$CGI->param("OS");
my %OS_Count=();
my $OS_Name;
my $count;

open(COUNT,"$countfile")||die"Can't open: $!\n";
flock(COUNT,LOCK_SH);
while(<COUNT>){
  if(/^$/){}else{
  ($OS_Name,$count)=split /=/;
  chomp($OS_Name);
  chomp($count);
  $OS_Count{$OS_Name}=$count;
}
flock(COUNT,LOCK_UN);
close COUNT;

if($OS_Type=~/Linux/ || $OS_Type=~/Windows/){
  if(exists $OS_Count{$OS_Type}){
    $OS_Count{$OS_Type}++;
  }
  &write(\%OS_Count);
  if($OS_Type=~/Windows/){
    $file="http://tstanley.perlmonk.org/QuizTaker32-V108.zip";
    $CGI->redirect($file);
  }elsif($OS_Type=~/Linux/){
    $file="http://tstanley.perlmonk.org/QuizTaker-V1.08.tar.gz";
    $CGI->redirect($file);
  }else{
    print $CGI->header();
    print $CGI->start_html('Try Again!');
    print $CGI->h1(-align=>'center','Incorrect OS!!');
    print $CGI->end_html();
  }
}elsif($OS_Type=~/Show/){
  print $CGI->header();
  print $CGI->start_html('Number of Downloads');
  
  foreach my $key(keys %OS_Count){
    print $CGI->h3("$key = $OS_Count{$key} downloads");
  }

  print $CGI->end_html();
}

sub write{
  my $Hash=shift;

  open(COUNT,"+<$countfile")||die"Can't open: $!\n";
  flock(COUNT,LOCK_EX);
  seek(COUNT,0,0);
  truncate(COUNT,0);
  
  foreach my $key (keys %$Hash){
    print COUNT "$key=$$Hash{$key}\n";
  }
  flock(COUNT,LOCK_UN);
  close COUNT;
}
[download]

[Tue Aug 21 19:54:01 2001] [error] [client 63.57.209.20] Premature end
+ of script
 headers: /home/tstanley/public_html/cgi-bin/download.cgi
Missing right bracket at /home/tstanley/public_html/cgi-bin/download.c
+gi line 68
, at end of line
syntax error at /home/tstanley/public_html/cgi-bin/download.cgi line 6
+8, at EOF
Execution of /home/tstanley/public_html/cgi-bin/download.cgi aborted d
+ue to comp
ilation errors.
[Tue Aug 21 19:54:11 2001] [error] [client 63.57.209.20] Premature end
+ of script
 headers: /home/tstanley/public_html/cgi-bin/download.cgi
Missing right bracket at /home/tstanley/public_html/cgi-bin/download.c
+gi line 68
, at end of line
syntax error at /home/tstanley/public_html/cgi-bin/download.cgi line 6
+8, at EOF 
Execution of /home/tstanley/public_html/cgi-bin/download.cgi aborted d
+ue to comp
ilation errors.
[Tue Aug 21 19:55:09 2001] [error] [client 63.57.209.20] Premature end
+ of script
 headers: /home/tstanley/public_html/cgi-bin/download.cgi
[download]

<A HREF="http://tstanley.perlmonk.org/cgi-bin/download.cgi?OS=Linux">L
+inux</A>
[download]

Your problem is in this part:

while(<COUNT>){
  if(/^$/){}else{
  ($OS_Name,$count)=split /=/;
  chomp($OS_Name);
  chomp($count);
  $OS_Count{$OS_Name}=$count;
}
[download]

You missed a bracket for the "else". I'd probably code this something like this:

while(<COUNT>){
    next if /^\s*#?\s*$/; # skip blank lines (the hash mark lets you a
+dd comments)
    chomp;
    ($OS_Name,$count)   = split /=/;
    $OS_Count{$OS_Name} = $count;
}
[download]

As for the "premature end of script headers", when you click links for Linux or Windows, you forgot to print the redirect :)

    print $CGI->redirect($file);
[download]

Cheers,
Ovid

Vote for paco!

Join the Perlmonks Setiathome Group or just click on the the link and check out our stats.