Closed Bug 92263 Opened 23 years ago Closed 23 years ago

Don't output SQL commands before the footer.


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




Bugzilla 2.16


(Reporter: CodeMachine, Assigned: justdave)




(1 file, 2 obsolete files)

The other day when there was problems with shadow database syncing on, 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
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.
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
Is it possible to spawn this stuff off as a separate process?
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 
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: Bugzilla 2.13 → 2.10
correcting version field lost in product move
Version: 2.10 → 2.13
Won't this break on windows?
Depends on: 104589
Keywords: patch, review
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
Attachment #59051 - Flags: review-
_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};\"";
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.
Attached patch Windows-compliant version (obsolete) — Splinter Review
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
Attachment #59051 - Attachment is obsolete: true
Need someone with Bugzilla on Win32 to verify if this actually works on Windows...
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?
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.
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.
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 doesn't exist in Windows, b) Windows could
care less about the shebang line, so the script has to be (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? :)
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 ;)
After a bit of experimentation and some google queries, I found a string that 

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

nul works, NUL: does not.
wouldn't that create a file called "nul" in the current directory?
uses NUL instead of NUL:.  Did my own research, and it backs up what's been
said here.
Attachment #68829 - Attachment is obsolete: true
Comment on attachment 72822 [details] [diff] [review]
Windows-compliant, take 2

If it works on windows, r=bbaetz
Attachment #72822 - Flags: review+
Comment on attachment 72822 [details] [diff] [review]
Windows-compliant, take 2

Presuming you're not supposed to close STDOUT and STDERR before reopening them,

Attachment #72822 - Flags: review+
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.151; previous revision: 1.150
Closed: 23 years ago
Resolution: --- → FIXED
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.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.