92263 - Don't output SQL commands before the footer.

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Matthew Tuck [:CodeMachine]]

The other day when there was problems with shadow database syncing on
mozilla.org, people (including me) were getting an SQL INSERT statement that was
an account creation, just before their footer.

This had the account's unencrypted password, as well as the person's email.  Now
we don't store cleartext passwords in 2.14, but we still shouldn't be outputting
this garbage.

I've been told that apparently it was trying to remove leftover shadow sync
commands.  That's all well and good, but it shouldn't be outputting those
commands.

Comment 1

[Dave Miller [:justdave]]

fix for this is to redirect stderr to /dev/null (or to a logfile) before 
executing the syncshadowdb command in PutFooter() (but AFTER spawning the second 
thread to run it in).  This may also help with the problem I see frequently where 
the browser sits there spinning its wheels for a while after the footer 
completes...  I think Apache is still watching both threads, and even though the 
parent completed, the other is still outputting to stdout/stderr so apache waits 
for it.

Comment 2

[Matthew Tuck [:CodeMachine]]

Is it possible to spawn this stuff off as a separate process?

Comment 3

[Dave Miller [:justdave]]

Matty: That's exactly what it's doing now.  However, since it's not detaching 
stderr, Apache is apparently still hanging on to it instead of releasing the 
session.

Comment 4

[Dave Miller [:justdave]]

correcting version field lost in product move

Comment 5

[Dave Miller [:justdave]]

Attachment: redirect output to /dev/null before running syncshadowdb

Comment 6

[Bradley Baetz (:bbaetz)]

Won't this break on windows?

Comment 7

[Zach Lipton [:zach]]

Comment on attachment 59051 [details] [diff] [review]
redirect output to /dev/null before running syncshadowdb

This is going to break windows, needs work. I'm about to paste some code in
from parrot (the backend runtime for perl6) that will do this on unix and
windows though it isn't pretty

Comment 8

[Zach Lipton [:zach]]

_run_command() should do it. You can call it like:

_run_command('dosomethingfun','STDOUT' => 'foo', 'STDERR; => 'bar');

# this kludge is an hopefully portable way of having
# redirections ( tested on Linux and Win2k )
sub _run_command {
  my( $command, %redir ) = @_;
  my( $redir_string ) = '';

  while( my @dup = each %redir ) {
    my( $from, $to ) = @dup;
    if( $to eq 'STDERR' ) { $to = "qq{>&STDERR}" }
    elsif( $to eq 'STDOUT' ) { $to = "qq{>&STDOUT}" }
    elsif( $to eq '/dev/null' ) { $to = ( $^O eq 'MSWin32' ) ?
                                      'qq{> NUL:}' : "qq{> $to}" }
    else { $to = "qq{> $to}" }

    $redir_string .= "open $from, $to;"
  }

  system "$^X -e \"$redir_string;system q{$command};\"";
}

Comment 9

[Dave Miller [:justdave]]

Ah, but the important part of that code to notice is that on Windows you use
NUL: instead of /dev/null.

The rest of the existing patch is still valid, no reason for the system call. :)
 The system call just gets around having to save and restore the filehandle.

Comment 10

[Dave Miller [:justdave]]

Attachment: Windows-compliant version

And actually, in this case, you don't even need to save/restore the filehandle,
because it's an exec in a fork, so control is never returning to the perl code
anyway.

Comment 11

[Dave Miller [:justdave]]

Need someone with Bugzilla on Win32 to verify if this actually works on Windows...

Comment 12

[John Tokash]

If this patch is supposed to run against 2.14.1, I'll run it on my win32 
install and test.  Also, what entry point excersizes this code?

Comment 13

[Dave Miller [:justdave]]

Hmm, this probably will apply to 2.14.1 with an offset...  that chunk of code
hasn't changed since then.  In order to test this, you'd need to tell
editparams.cgi that you were running a shadow database, and probably have
something corrupted in the shadow database (such as not having one ;) in order
to get the shadow update code to spew an error.  You can probably test this by
trying to activate the shadow database feature without actually creating a
shadow database (and not applying this patch yet).  Make a change to a bug, and
then look for error messages to show up either right before or right after the
footer at the bottom of all the process_bug pages.  Then apply the patch and see
if those error messages go away.

Comment 14

[John Tokash]

Applied the patch to a win32 install and stderr is reproducibly getting 
appended to the bottom of the returned document.  I don't have any suggestions 
for an alternative to "NUL:" except for win32 folks who don't want ANY warnings 
or stderr returned; changing the appmapping for perl from perl %s %s to perl -X 
%s %s makes stderr dissappear.

The shadowdb code doesn't seem to work on my win32 box, so the errors are more 
like: "Couldn't do db copy at syncshadowdb line 178."  No SQL statements are in 
the error list.  But if syncshadowdb does work on some win32 boxes, maybe the 
error returned there would carry SQL statements.

Comment 15

[Dave Miller [:justdave]]

OK, John did some testing which he mailed to me privately (thanks, John!) and
it's quite evident that using a shadow database as a whole is completely horked
on Windows.  a) it has no clue where the MySQL executables are because the
"which" command run by checksetup.pl doesn't exist in Windows, b) Windows could
care less about the shebang line, so the script has to be syncshadowdb.pl (with
the pl extension on the end) in order to run it.  Even when he fixed those two
issues, it was so full of warnings it wasn't even funny.

End result: I don't think it's worth even attempting to make this one line of
code work on Windows right now, since the shadowdb stuff as a whole needs to be
fixed in order to work on windows, and that's Another Bug. :)

With shadowdb disabled, Bugzilla will never run this line of code, and you can't
safely enable it on Windows at this point.

zach/bbaetz: either of you care to recind your "needs-work" for the first patch,
since it's obvious it doesn't matter? :)

Comment 16

[Bradley Baetz (:bbaetz)]

I'm fine with that, except that I'd like to check that NUL: works, cause I
copied that line for template outputs for precompilation - its easier then
outputting to an empty string.

If NUL works, then just use the second version. Its not going to hurt anything,
and its technically more correct.

Don't bother filing a bug on the win32 shadowdb stuff - this is one more reason
to use replication instead ;)

Comment 17

[John Tokash]

After a bit of experimentation and some google queries, I found a string that 
works.

Change it to:
my $redir = ($^O =~ /MSWin32/i) ? "nul" : "/dev/null";

nul works, NUL: does not.

Comment 18

[Dave Miller [:justdave]]

wouldn't that create a file called "nul" in the current directory?

Comment 19

[Dave Miller [:justdave]]

Attachment: Windows-compliant, take 2

uses NUL instead of NUL:.  Did my own research, and it backs up what's been
said here.

Comment 20

[Bradley Baetz (:bbaetz)]

Comment on attachment 72822 [details] [diff] [review]
Windows-compliant, take 2

If it works on windows, r=bbaetz

Comment 21

[Gervase Markham [:gerv]]

Comment on attachment 72822 [details] [diff] [review]
Windows-compliant, take 2

Presuming you're not supposed to close STDOUT and STDERR before reopening them,
r=gerv.

Gerv

Comment 22

[Dave Miller [:justdave]]

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.151; previous revision: 1.150
done

Comment 23

[J. Paul Reed [:preed]]

Checked in to the 2_14_1-BRANCH, per a suggestion from MattyT on reviewers@.

As the comment says, applied cleanly to the branch (save a small offset notice),
so r/r2 = me.