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

This is my second attempt at a full rewrite of this random password generator. This script *should* open $saved to check if our password has already been used, if so it should stay in the loop until it finds one that's new.

I spent 2 full days trying to learn how to open and read files and I think I've got the hang of it so I'm assuming that's not the problem. My guess is it's the loop since I'm still fuzzy about those.

Like all my scripts, I don't get any reported errors anywhere, it just doesn't load. I get printed to screen "Your password is:" without your new password.

Since this is becoming such a hassle to get this one script to work, I'd really like to know what I'm doing wrong instead of just a fix to this. Any help would be much appreciated.

#!/usr/bin/perl
use strict;
use warnings;
use CGI;
use CGI::Carp 'fatalsToBrowser';

my $q = CGI->new;
print $q->header;

my $saved = "savedpasswords.txt";
my @chars = ( "A" .. "Z", "a" .. "z". 0 .. 9 qw( ! @ $ % ^ & *) );

open(SAVEDPW, "< $saved") or die $!;
flock(SAVEDPW, 1) or die $!;
my @stored = <SAVEDPW>;
close(SAVEDPW);

chomp(@stored);

my $pw;

# attempt at looping
unless ($_ ne @stored) {
$pw = join(@chars[map{rand @chars} (1..17)];
}

# loop is over, we have a new password
print "Your new password is: $_";

open(SAVEDPW, ">> $saved") or die $!;
flock(SAVEDPW, 2) or die $!;
print SAVEDPW "$_\n";
close(SAVEDPW);

[download]


"Age is nothing more than an inaccurate number bestowed upon us at birth as just another means for others to judge and classify us"

sulfericacid

Replies are listed 'Best First'.
Re: Variable still not holding contents
by jdporter (Paladin) on Jan 07, 2003 at 06:05 UTC
    First off, you have a syntax/typo problem here:   my @chars = ( "A" .. "Z", "a" .. "z". 0 .. 9 qw( ! @ $ % ^ & *) ); That should be a comma after the "z", not a dot; and you also need a comma after the 9.

    Now let's look at your loop: 
      # attempt at looping
  unless ($_ ne @stored) {
    $pw = join(@chars[map{rand @chars} (1..17)];
  }

    [download]
    Well, that ain't a loop at all. "unless" is not a looping control word. You probably want "while" or "until". In pseudocode, you probably mean   until ( $_ not in @stored ) or the equivalent,   while ( $_ in @stored ) Now, as for how you're checking whether $_ is in @stored, a simple "eq" or "ne" is not going to get it. You can use the grep function, which, in scalar context, returns the number of "matches". Like so:   $n_found = grep { $pw eq $_ } @stored; In the condition of a while loop, it would look like this:   while ( grep { $pw eq $_ } @stored ) The other mistake you made was to test $_ in the condition, rather than $pw; but that was probably a simple typo.

    However, we're not out of the woods yet, because, as it stands, the while condition will always test false on the first iteration, for the simple reason that $pw has no value yet, and thus will never be found in the list. (Unless, of course, and undef somehow got into the list...)

    One way to account for that is to give $pw its first value before entering the loop test for the first time: 
      $pw = join(@chars[map{rand @chars} (1..17)];
  while ( grep { $pw eq $_ } @stored ) {
    $pw = join(@chars[map{rand @chars} (1..17)];
  }

    [download]
    But that's ugly. An alternative is to switch the loop style to use the test-at-end form of while, aka "do-while": 
      do {
    $pw = join(@chars[map{rand @chars} (1..17)];
  }
  while ( grep { $pw eq $_ } @stored );

    [download]

    jdporter
    The 6th Rule of Perl Club is -- There is no Rule #6.

[reply]
[d/l]
[select]
Re: Variable still not holding contents
by virtualsue (Vicar) on Jan 07, 2003 at 11:00 UTC
    This site has a big collection of Tutorials. You may find it helpful to take the time to look through them and study the ones which cover areas that you don't understand. For your immediate problem, see: I also suggest that you get in the habit of testing each little piece of your program from start to finish and fixing each piece as you go along, rather than assembling a lot of stuff and then hoping to get it to all work somehow. This way getting a small project like your password generator to work will take minutes to hours rather than days. You will eventually be able to write larger chunks of code at a time as you gain confidence in what works and what doesn't.
[reply]
Re: Variable still not holding contents
by djantzen (Priest) on Jan 07, 2003 at 06:19 UTC

    If you want to loop, you need to use a loop construct, not a conditional statement (and especially not a double-negative conditional :) I take it you want to search @stored for a submitted password, (is it really stored in $_?), and if you fail to find it then you generate a new one. Try: 

    my $submitted_pw = $_;
my $pw;
foreach my $stored_pw (@stored) {
    if ($submitted_pw eq $stored_pw) {
      $pw = $stored_pw and last; #exit the loop if we found a match
    }
} 
# If $pw wasn't set in the loop, create a new password
$pw ||= join(@chars[map{rand @chars} (1..17)];

    [download]

    Another way: 

    my $submitted_pw = $_;
my $pw = grep($submitted_pw eq $_, @stored) ? $submitted_pw : join(@ch
+ars[map{rand @chars} (1..17)];

    [download]

    Update: well there I go answering the wrong question again. How about this: 

    my %lookup_pwds;
$lookup_pwds{$_}++ for (@stored};
do {
    $pw = join(@chars[map{rand @chars} (1..17)];
} while ($lookup_pwds{$pw});

    [download]
[reply]
[d/l]
[select]
Re: Variable still not holding contents
by graff (Chancellor) on Jan 07, 2003 at 06:57 UTC
    Looks like jdporter covered the major points, except for the fact that your use of "join()" (with the embedded "map" and "rand") won't work, either.

    Slow things down a bit, give yourself some space, and use a loop to generate the candidate password -- put it in a subroutine, maybe, to keep it clear: 

    ...
my $pw;
do {
   $pw = make_new_pw();
} while ( grep { $pw eq $_ } @stored );
...

# update: moved the declaration of @chars into the subroutine,
# where it belongs:

sub make_new_pw
{
    my @chars = ( "A" .. "Z", "a" .. "z", 0 .. 9, qw( ! @ $ % ^ & *) )
+;
    my $newpw = "";
    for ( 1 .. 17 ) {
        $newpw .= $chars[ rand @chars ];
    }
    return $newpw;
}

    [download]
    One closing tip: I've seen people get really bolixed because they are sent a password like "lXIg1v0WO" -- given the chance to control which letters are available to go into a random password, I would tend to leave "I1l" and "0O" out of the set, because they are too easily confused and misread.
[reply]
[d/l]
      Good catch!
      Actually, the only problem with his code as given was the missing join character as the first argument to join. Inserting a null string:   $pw = join('',@chars[map{rand @chars} (1..17)]; This works, because map{rand @chars} (1..17) does indeed generate a list of 17 indices, each in the proper range for the @chars array. (They're floating point, but they get intified when used as indices.)
      That said, it's not how I would have done it. I would have written   $pw = join '', map { $chars[ rand @chars ] } 1..17;

      jdporter
      The 6th Rule of Perl Club is -- There is no Rule #6.

[reply]
[d/l]
[select]
        ...the only problem with his code as given was the missing join character...

        And then there's the missing close paren that needs to be added after the close square bracket. Anyway, I agree that you're latter approach is clean and easy enough -- still, I think that having a subroutine with a name that indicates what the **** is going on is a really nice feature.

[reply]
Re: Variable still not holding contents
by Zaxo (Archbishop) on Jan 07, 2003 at 16:19 UTC

    jdporter and the others have showed you what was giving unexpected results on every run. There is one more error that will occasionally give you problems, perhaps without your ever knowing it.

    You have a 'race condition' on your savedpasswords.txt file. That means that two instances (call tham A and B) of your script running at the same time can give you a corrupt save file. If the sequence

    1. A opens to read,
    2. B opens to read,
    3. B opens to write,
    4. A opens to write.
    occurs, B fails to check its generated password against A's.

    By opening to append rather than truncate when you write, you avoided the much more serious error of B's result being not recorded after the above sequence. In either case the probability of error is small, but it is a structural problem that can do you serious injury in other circumstances. The worst that can happen here is that two users have the same hashed passwd, with mischief available to the first one to notice.

    To fix the problem, open the file only once, in '+>>' mode, and do not close it again until you have written the new password.

    I'd like to second virtualsue's more general advice, it is the best way I know to get comfortable with a language.

    After Compline,
    Zaxo

[reply]
Re: Variable still not holding contents
by PodMaster (Abbot) on Jan 07, 2003 at 07:18 UTC
    Please use more descriptive die messages, and always use Fcntl. 
    use Fcntl qw[ :flock ];
flock(SAVEDPW, LOCK_SH) or die "can't lock $saved:$!";

    [download]

    MJD says you can't just make shit up and expect the computer to know what you mean, retardo!
    ** The Third rule of perl club is a statement of fact: pod is sexy.

[reply]
[d/l]

Back to Seekers of Perl Wisdom