Last Comment Bug 92263 - Don't output SQL commands before the footer.
: Don't output SQL commands before the footer.
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: 2.13
: All All
P1 normal (vote)
: Bugzilla 2.16
Assigned To: Dave Miller [:justdave] (
: default-qa
Depends on: 104589
  Show dependency treegraph
Reported: 2001-07-25 09:35 PDT by Matthew Tuck [:CodeMachine]
Modified: 2012-12-18 20:46 PST (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

redirect output to /dev/null before running syncshadowdb (745 bytes, patch)
2001-11-24 11:38 PST, Dave Miller [:justdave] (
zach: review-
Details | Diff | Splinter Review
Windows-compliant version (825 bytes, patch)
2002-02-10 19:33 PST, Dave Miller [:justdave] (
no flags Details | Diff | Splinter Review
Windows-compliant, take 2 (823 bytes, patch)
2002-03-06 11:29 PST, Dave Miller [:justdave] (
bbaetz: review+
gerv: review+
Details | Diff | Splinter Review

Description User image Matthew Tuck [:CodeMachine] 2001-07-25 09:35:29 PDT
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
Comment 1 User image Dave Miller [:justdave] ( 2001-07-25 11:26:01 PDT
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 User image Matthew Tuck [:CodeMachine] 2001-08-09 10:34:08 PDT
Is it possible to spawn this stuff off as a separate process?
Comment 3 User image Dave Miller [:justdave] ( 2001-08-09 17:40:06 PDT
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 
Comment 4 User image Dave Miller [:justdave] ( 2001-08-28 01:33:52 PDT
correcting version field lost in product move
Comment 5 User image Dave Miller [:justdave] ( 2001-11-24 11:38:26 PST
Created attachment 59051 [details] [diff] [review]
redirect output to /dev/null before running syncshadowdb
Comment 6 User image Bradley Baetz (:bbaetz) 2002-02-01 01:56:26 PST
Won't this break on windows?
Comment 7 User image Zach Lipton [:zach] 2002-02-10 19:15:04 PST
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 User image Zach Lipton [:zach] 2002-02-10 19:17:57 PST
_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 User image Dave Miller [:justdave] ( 2002-02-10 19:26:47 PST
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 User image Dave Miller [:justdave] ( 2002-02-10 19:33:02 PST
Created attachment 68829 [details] [diff] [review]
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
Comment 11 User image Dave Miller [:justdave] ( 2002-02-11 23:13:53 PST
Need someone with Bugzilla on Win32 to verify if this actually works on Windows...
Comment 12 User image John Tokash 2002-03-04 17:53:04 PST
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 User image Dave Miller [:justdave] ( 2002-03-04 18:03:45 PST
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 User image John Tokash 2002-03-04 18:48:20 PST
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 User image Dave Miller [:justdave] ( 2002-03-04 18:51:17 PST
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? :)
Comment 16 User image Bradley Baetz (:bbaetz) 2002-03-04 19:22:44 PST
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 User image John Tokash 2002-03-04 19:47:17 PST
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.
Comment 18 User image Dave Miller [:justdave] ( 2002-03-04 19:53:15 PST
wouldn't that create a file called "nul" in the current directory?
Comment 19 User image Dave Miller [:justdave] ( 2002-03-06 11:29:45 PST
Created attachment 72822 [details] [diff] [review]
Windows-compliant, take 2

uses NUL instead of NUL:.  Did my own research, and it backs up what's been
said here.
Comment 20 User image Bradley Baetz (:bbaetz) 2002-03-09 14:55:30 PST
Comment on attachment 72822 [details] [diff] [review]
Windows-compliant, take 2

If it works on windows, r=bbaetz
Comment 21 User image Gervase Markham [:gerv] 2002-03-29 10:01:53 PST
Comment on attachment 72822 [details] [diff] [review]
Windows-compliant, take 2

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

Comment 22 User image Dave Miller [:justdave] ( 2002-03-31 15:04:00 PST
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.151; previous revision: 1.150
Comment 23 User image J. Paul Reed [:preed] 2002-05-26 17:54:18 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.