185330 - Checksetup.pl console input doesn't work on Win32 (linefeeds)

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Jouni Heikniemi]

When trying to enter an administrator password on Win32, I can enter the pwd but
when hitting enter, it doesn't give an error message but rather just reprints
the prompt. I've been unable to get this to work. Luckily Joel implemented the
non-interactive checksetup, so that I could use it. ;-)

Comment 1

[Rune Skaug]

It works after an additional chop(), I guess there is something about the line 
break on Windows being different from the ***x one...?

Comment 2

[Tobias Zang]

My output from BZ 2.17.3:

Enter the e-mail address of the administrator: [e-mail]
You entered '[e-mail]\r'.  Is this correct? [Y/n] y
Enter the real name of the administrator: Tobias Zang
[Wed Feb 12 12:02:05 2003] D:\bugzilla-2.17.3\checksetup.pl: No such signal: SIG
HUP at D:\bugzilla-2.17.3\checksetup.pl line 3737, <STDIN> line 5.
Enter a password for the administrator account: Enter a password for the adminis
trator account: Enter a password for the administrator account: ...

Inserted trace in checksetup.pl and gave password 'abc123':
---
    			print FILE "Before chomp";
			print FILE "<<<$pass1>>>";
        chomp $pass1;
    			print FILE "After chomp";
			print FILE "<<<$pass1>>>";
creates

C:\>cat myfile
Before chomp<<<abc123
>>>After chomp<<<abc123
C:\>
C:\>od -c myfile
0000000000     B   e   f   o   r   e       c   h   o   m   p   <   <   <   a
0000000020     b   c   1   2   3  \r  \r  \n   >   >   >   A   f   t   e   r
0000000040         c   h   o   m   p   <   <   <   a   b   c   1   2   3  \r
0000000060     >   >   >
0000000063

The chomp only removes the '\r\n' from '\r\r\n'.

The reason is either the chomp command which leaves a CR (\r) at the string's 
end or the may be too strict while clause:
   3746   while( $pass1 eq "" || $pass1 !~ /^[a-zA-Z0-9-_]{3,16}$/ ) {
I fixed this with adding the '\r' to the allowed chars
   3746   while( $pass1 eq "" || $pass1 !~ /^[a-zA-Z0-9-_\r]{3,16}$/ ) {

Since I'm a Perl newbee you have to decide whether the above syntax is correct, 
whether the \r in the DB is a problem, or whether an additional 'chop' is the 
way.

Comment 3

[Jason Pyeron]

This works for me
Microsoft Windows 2000 [Version 5.00.2195]
Bugzilla 2.17.4
Perl v5.8.0 built for cygwin-multi-64int

I will check branch 2.17.1 on Wednesday

Comment 4

[Jouni Heikniemi]

Well, this needs to get done some day. I wish I would understand why chomp works
sometimes and sometimes not... Going to propose some chewing gum to make sure.

Comment 5

[Jouni Heikniemi]

Attachment: manual removal of \r's

Let's do this the hard way then. I've seen this problem reported elsewhere too,
and this problem also plagues the Cygwin port every now and then. 

The patch adds s/\r//:s, removing the unnecessary line feeds. And yes, allowing
them to go the db would be a problem at some point.

Comment 6

[Zach Lipton [:zach]]

Comment on attachment 128575 [details] [diff] [review]
manual removal of \r's

This is a bit of a cludge, but it works and we need it for win32 support.
r=zach if you (or whoever checks this in) adds a quick comment to the s/\r//
lines explaining the purpose for doing this.

Comment 7

[Dave Miller [:justdave]]

hmmm...
looking at the Perl docs, that MAY be a bug in ActiveState Perl.

looks like $/ is supposed to be set to whatever the system uses for newline,
even if it's multicharacter, according to the docs.  And whatever's in $/ is
what chomp() is supposed to remove.

Doing |if ($^O =~ /win32/i) { $/ = "\r\n" }| somewhere early in the script would
probably fix it. (and would be less confusing in the remaining code)

Can someone with Windows give that a try and report back?

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 128575 [details] [diff] [review]
manual removal of \r's

Denying review based on my previous comment.  I *may* reverse this based on the
discussion.  I just want to discuss it a little more first.

Comment 9

[Dave Miller [:justdave]]

It was pointed out on IRC that ActiveState and CygWin would probably both return
Win32 for $^O, and CygWin probably uses \n instead of \r\n for newline...

Maybe the best way to go about this is just prompt the user to press Enter
before asking the other questions, and whatever we get from that input line
(sanity-checking for non-[\r\n] characters of course) we stuff into $/

Comment 10

[Jason Pyeron]

hope thi helps...

C:\>perl -e'print $^O'
cygwin
C:\>perl -v

This is perl, v5.8.0 built for cygwin-multi-64int

Copyright 1987-2002, Larry Wall

Perl may be copied only under the terms of either the Artistic License or the
GNU General Public License, which may be found in the Perl 5 source kit.

Complete documentation for Perl, including FAQ lists, should be found on
this system using `man perl' or `perldoc perl'.  If you have access to the
Internet, point your browser at http://www.perl.com/, the Perl Home Page.


C:\>uname -a
CYGWIN_NT-5.0 MrLaptop 1.3.22(0.78/3/2) 2003-03-18 09:20 i686 unknown unknown 
Cygwin

C:\>ver

Microsoft Windows 2000 [Version 5.00.2195]

C:\>

Comment 11

[Jason Pyeron]

re comment 7, that looks like a good test

re comment 9, cygwin is the platform, so it is not 'win32'

also I think it is better to handle inside the code rather than expose it to 
the user (admin) so no just press enter.

I would rather have a table of exceptions in  a .PM file, and consult it, 
update it, etc. and if neccessary utility functions.

Comment 12

[Jouni Heikniemi]

I'll try to test and create another along the lines of comment 7, but I'm
terribly busy right now (just back from a 2-week vacation) - so if somebody has
the interest to take this, feel free.

Comment 13

[John Stosh Muczynski]

Note that it isn't just the password that gets messed up.  It is all input from 
STDIN after checking the version on CGI.  The extra \r are showing up because 
CGI sets binmode on STDIN.

As already noted, setting $/ to "\r\n" allows chomp to remove the \r when it 
removes the \n.  BUT, you would only want to do this on a system (like windows) 
that uses crlf translation.  Setting binmode at the top of the file might avoid 
all confusion.
   CORE::binmode(STDIN);
   $/ = "\r\n";
The discussion in 
http://www.perldoc.com/perl5.8.0/pod/perlport.html#Nearly-all-of-Perl-already-
is-portable
makes it clear that setting $/ is an appropriate solution.  There does not 
appear to be any way to reverse the effect of setting binmode.  That is truely 
the pity.  (Please someone prove me wrong on this point.)

(If you patch checksetup.pl for this, perhaps you'd also like to patch the 
SIGHUP stuff.)

Comment 14

[John Stosh Muczynski]

Hmm... I wonder if CGI could be changed to fix this problem.

Comment 15

[Piotr Lakomy]

On my machine 
  This is perl, v5.6.1 built for MSWin32-x86-multi-thread
  Binary build 633 provided by ActiveState
  $CGI::VERSION = 2.91
It's also problem with CGI.

CGI internally (in order to handle binary uploads) does:

  if ($OS=~/^(WINDOWS|DOS|OS2|MSWin|CYGWIN)/) {
    $CGI::DefaultClass->binmode(main::STDIN);
  }

My workaroung was in checksetup.pl line around 263:

  if ($^O =~ /^(WINDOWS|DOS|OS2|MSWin|CYGWIN)/) {
      binmode STDIN, ":crlf";
  }

Comment 16

[John Stosh Muczynski]

http://www.perldoc.com/perl5.8.0/pod/perlport.html
http://www.perldoc.com/perl5.8.0/lib/PerlIO.html
and
http://www.perldoc.com/perl5.8.0/pod/func/binmode.html

Say that :raw is not the simply inverse of :crlf 

So, the suggested
  if ($^O =~ /^(WINDOWS|DOS|OS2|MSWin|CYGWIN)/) {
      binmode STDIN, ":crlf";
  }
may be fine, but I suggest the following:

my ($OS, $needs_binmode);
# CGI sets binmode, so this is the fixup
# The following paragraphs were duplicated from CGI.pm
unless ($OS) {
    unless ($OS = $^O) {
	require Config;
	$OS = $Config::Config{'osname'};
    }
}
if ($OS =~ /^MSWin/i) {
  $OS = 'WINDOWS';
} elsif ($OS =~ /^VMS/i) {
  $OS = 'VMS';
} elsif ($OS =~ /^dos/i) {
  $OS = 'DOS';
} elsif ($OS =~ /^MacOS/i) {
    $OS = 'MACINTOSH';
} elsif ($OS =~ /^os2/i) {
    $OS = 'OS2';
} elsif ($OS =~ /^epoc/i) {
    $OS = 'EPOC';
} elsif ($OS =~ /^cygwin/i) {
    $OS = 'CYGWIN';
} else {
    $OS = 'UNIX';
}

# Some OS logic.  Binary mode enabled on DOS, NT and VMS
$needs_binmode = $OS=~/^(WINDOWS|DOS|OS2|MSWin|CYGWIN)/;

if ($needs_binmode)
{
   CORE::binmode(STDIN);
   # CGI sets binmode, so this is the fixup
   $/ = "\r\n";
}

(This makes it clear that the script is now running in binmode.)

This still leaves the question of why the binmode in CGI.pm wasn't hurting 
before but clearly hurts now...

Comment 17

[Jason Pyeron]

this activestate stuff is for the birds, but that being said
I will have one of our machines done up with win2kpro activestate current
bz tip and test.


google returns many issues with activestate's activeperl and crlf issues...

Comment 18

[Jouni Heikniemi]

[e-mail], re your comment 16: Why do you suggest that? The proposal
in comment 15 is just two lines, and as such, more readable than the pageful of
code. I'm inclined to make the shortest patch possible for this. Writing a heavy
OS mapping doesn't feel like a priority at this point.

Anyway, please present whatever reasoning you have for the longer fix. I'll come
up with a patch one of these days, but I'd like to hear your opinion first.

Comment 19

[Jouni Heikniemi]

Attachment: v2

Well, I had the time so... This patch fixes it for me. Setting binmode :crlf
didn't work as expected, as after that no enter hits worked. Well, intuitively,
setting the input line terminator should suffice. I tested this on |ActiveState
perl, v5.8.0 built for MSWin32-x86-multi-thread| and |perl, v5.8.0 built for
i386-linux-thread-multi| (RH8). A cygwin test would be useful, but I don't have
a reliable testing platform handy. Jpyeron, can you test the patch on cygwin?

I'll ask for reviews soon, but I'll wait for Johnstosh's comments and the
Cygwin test results first. We'll miss the 2.17.5 train anyway.

Comment 20

[Jouni Heikniemi]

Oopps... [e-mail], please check the previous a couple of comments.
Didn't notice you weren't ccd. All others, my apologies for the spam.

Comment 21

[John Stosh Muczynski]

I thought I was CC'd as well.  Guess I biffed it.

The answer to your question seems complicated.
The best patch would be to simply undo what CGI has done, but
that isn't possible.

A robust solution would be do modify only the platforms that CGI is
modifying, so I suggested the long patch because it is an exact duplicate
of the CGI code.  

Of course, duplicating CGI code puts us in the position of needing to mirror 
changes in CGI.  The most wonderful of solutions would be to fix this problem 
by getting CGI modified.  If CGI changed $/ when changing binmode on STDIN, 
then that would appear to fix checksetup.pl, but what would it do to runtime 
execution when CGI reads STDIN?

(conclusion)
So I think your patch in comment 19 would work fine.


Question on the side: Why do you have the text "WINDOWS" in
   if ($^O =~ /^(WINDOWS|DOS|OS2|MSWin|CYGWIN)/i) {
I ask because based on the CGI code, $^O doesn't seem to ever contain
"WINDOWS".  I don't think it hurts.  The CGI code has a 
similar issue in searching for MSWin in $OS when $OS will
clearly never contain MSWin...

By the way, I appreciate your use of \015\012 instead of \r\n.
CGI has the following code to handle cr/lf issues.  

# Define the CRLF sequence.  I can't use a simple "\r\n" because the meaning
# of "\n" is different on different OS's (sometimes it generates CRLF, 
sometimes LF
# and sometimes CR).  The most popular VMS web server
# doesn't accept CRLF -- instead it wants a LR.  EBCDIC machines don't
# use ASCII, so \015\012 means something different.  I find this all
# really annoying.
$EBCDIC = "\t" ne "\011";
if ($OS eq 'VMS') {
  $CRLF = "\n";
} elsif ($EBCDIC) {
  $CRLF= "\r\n";
} else {
  $CRLF = "\015\012";
}

But it isn't clear if CGI sets BINMODE on any EBCDIC systems.

Another aside: For robustness, may I suggest setting BINMODE when you 
set $\ and to do both AFTER the code that invokes CGI?  
That way if CGI is later modified to undo binmode, then 
checksetup.pl will still work.

Thanks for asking for my opinion.  I'll certainly apply any patches you post 
while I (slowly) do my testing.  I presume that noone has yet figured out why 
this problem has only recently appeared.

-Stosh

Comment 22

[Jouni Heikniemi]

I'm afraid of going down the route of mirroring CGI changes. Although I wouldn't
think CGIs operation is bound to change often, but still. For me, a working fix
is a fix good enough -- perfection is too hard to reach on this front. Changing
CGI also sounds like a reasonably heavy process. Even if we do get it changed,
requiring a brand new version of CGI just for this issue simply doesn't cut it
for me.

>Question on the side: Why do you have the text "WINDOWS" in
>   if ($^O =~ /^(WINDOWS|DOS|OS2|MSWin|CYGWIN)/i) {
>I ask because based on the CGI code, $^O doesn't seem to ever contain
>"WINDOWS".  I don't think it hurts.  The CGI code has a 
>similar issue in searching for MSWin in $OS when $OS will
>clearly never contain MSWin...

It's there because I pasted the conditional line from comment 15. I agree that
sucks. :-) Truth be told, that's probably unnecessary, and most of the previous
Windows identification has relied on checking MSWin only. Looking at it that
way, checking for only "MSWin" might be more consistent; DOS and OS2 are
probably out of our scope anyway.

Well. I've been meaning to do this before, but I'll do it now: filed bug 224588
on unifying OS checks throughout Bugzilla. That will take care of this once and
for all.

I'll make another patch before requesting for reviews, trimming out the
DOS|OS2|Windows stuff, but I'll need to see what should I do with Cygwin first
anyway. IIRC, Cygwin has an install-time option of using either dos or unix
linefeeds by default. Do I remember correctly? If so, do you guys think it's
possible to have some cygwin installations where it would be correct to set the
crlf and some where it wouldn't? If so, this is going to be hard...

I am considering the alternative of storing $/ before loading CGI and reapplying
it thereafter. Do any of you find a problem with that (theoretically, haven't
tested that yet)? This solution has the potential to fix the issue even with
Cygwins using CRLF. However, I don't think restoring the binmode settings for
stdin is doable anyway, so it's not going to become perfect anyway.

>Another aside: For robustness, may I suggest setting BINMODE when you 
>set $\ and to do both AFTER the code that invokes CGI?  
>That way if CGI is later modified to undo binmode, then 
>checksetup.pl will still work.

Hmm, do you mean $/ instead on $\ (they are two different variables, yay Perl)?
Probably yes.

I'll experiment a bit more with the binmode settings and try to move the code
around. Too bad I don't have a working cygwin Perl installation available. :-(

Comment 23

[Jouni Heikniemi]

Forgot to answer this last night:

>I presume that noone has yet figured out why 
>this problem has only recently appeared.

I believe it's because we started using the CGI module - probably bug 147833
checked in on 2002/10/26. That's not so much before this bug was filed.

Comment 24

[Jouni Heikniemi]

Attachment: v3

All right... I've tested various approaches on this one. It's interesting,
because restoring the original $/ value after loading CGI doesn't do what you'd
expect - the input still doesn't work. It's probably got something to do with
binmode settings, but I was unable to reverse them properly.

The patch I'm attaching is the best fix I can come up with right now. It works
for me and doesn't touch the cygwin platform. If cygwinists find problems, we
can of course fix them separately, but at least according to jpyeron's comment
10, it works right now. The lf install-time setting might change this, but I
haven't heard of any problems insofar. Dave's issues in comment 9 re cygwin
returning Win32 are addressed by comment 11; this checks only for MSWin in $^O.

Comment 25

[Dave Miller [:justdave]]

Comment on attachment 134769 [details] [diff] [review]
v3

looks painless enough.	I don't have Windows to test on, but I'll r= on the
basis that I know it won't break *nix.

Comment 26

[Vlad Dascalu]

jth, *cough* check-in... *cough* :-)

Comment 27

[Jouni Heikniemi]

Sorry for the delay. After Dave gave his r=, we discussed this further on IRC.
We suspect the fix for bug 226673 changed the conditions for this (we used to
load CGI twice, once in version checks and once for Carp, which we pulled
through RelationSet). So, I came around to think that a better fix might
actually be to retry just restoring $/ (see comment 24 for the previous
attempt), but then came the Xmas and I haven't been able to delve into this. 

Thinking of it now, I can't see why the restoring would turn out a lot better; I
believe |($^O eq 'MSWin') --> ($/ == "\015\012")| to be always true (in the
start of the application), so this fix is probably just as good. I'll just check
it in then.

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.256; previous revision: 1.255
done

Comment 28

[Jason Pyeron]

cygwin is still happy.
Good Job.