Last Comment Bug 225359 - Drawing dependency graphs is broken on Win32
: Drawing dependency graphs is broken on Win32
Status: RESOLVED FIXED
[needed for Win32bz]
:
Product: Bugzilla
Classification: Server Software
Component: Reporting/Charting (show other bugs)
: 2.17.5
: All All
: -- major (vote)
: Bugzilla 2.18
Assigned To: Byron Jones ‹:glob›
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-11-11 12:12 PST by Jouni Heikniemi
Modified: 2012-12-18 20:46 PST (History)
4 users (show)
justdave: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
remove open-pipe from showdependencygraph.cgi (1.26 KB, patch)
2004-05-28 08:48 PDT, Byron Jones ‹:glob›
no flags Details | Diff | Splinter Review
remove open-pipe from showdependencygraph.cgi v2 (1.16 KB, patch)
2004-05-28 08:51 PDT, Byron Jones ‹:glob›
jouni: review-
Details | Diff | Splinter Review
remove open-pipe from showdependencygraph.cgi v3 (1.40 KB, patch)
2004-06-14 05:48 PDT, Byron Jones ‹:glob›
jouni: review+
Details | Diff | Splinter Review

Description Jouni Heikniemi 2003-11-11 12:12:23 PST
In a classic episode of "commit now, let's fix this postcheckin", bug 197153
committed a fix that broke dependency charting on Win32. Actually, this was
already mentioned in bug 197153 comment 9. 

It's another simple use of pipe-open -- now we just need to come up with a way
to fix this.
Comment 1 Jouni Heikniemi 2003-11-11 12:13:07 PST
Oh and Bradley, if you want to take this, I don't mind at all. :-)
Comment 2 Jouni Heikniemi 2003-12-06 06:30:47 PST
Bbaetz, in bug 197153 comment 8 you say:

>I have a half patch for this. The problem is that we use -o for the dot output
>for local dot, and you can't generate a name for that w/o a small race
>condition. (Well, you could give it a pty for the fileno, I guess, and I think
>that there are perl modules to emulate that for non-unix, but....)

What do you mean by "you can't generate a name for that w/o a small race
condition"? How does piping the information through Bugzilla CGI aid in this sense?
Comment 3 Bradley Baetz (:bbaetz) 2003-12-06 15:57:50 PST
See
http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html,
7.10.1.2.

Someone can add a symlink between the time that the filename is checked for
existance, and the time that its actually opened.

If you use piping, then theres no file to use.
Comment 4 Jouni Heikniemi 2003-12-20 23:37:06 PST
Ok, an update on my findings so far:

This doesn't work because showdepgraph.cgi tries to read the PNG file from dot
through a pipe. Pipe open support is not implemented on ActiveState Perl. Ok, so
next, we have three workaround avenues:

1) Start using IPC::Run for executing the dot, thus giving us an emulation of
pipe opens. I've been toying with this, and I seem to have a surprising amount
of nastly little problems when executing stuff under taint mode. Temp files are
needed for piping simulation, $ENV{'TEMP'} is tainted and nasty other stuff. I
believe this can be made to work, I'm just not certain of how extensive codebase
 changes are we going to require.

2) Start using Graphviz perl module instead of directly running a dot binary.
This would probably work, but there are two issues:
  - There is no ActiveState PPM for GraphViz, so installation has to be made
    through CPAN. This is messy.
  - GraphViz has a nasty set of dependencies, ranging from IPC::Run to 
    Math::Bezier and so on - so we'll end up requiring IPC::Run anyway. 
    So far, I haven't tested whether GraphViz module internally suffers from
    the same issues as I encountered in 1); at least the IPC::Run syntax is
    close to being equal. I'd believe the problems are there.

3) Undo the security fix made in bug 197153 for Win32. This is a trivial fix 
   and will make it work with minimal effort. It shouldn't be a security issue
   as there is no concept of symlinks on Windows platforms. But the fix is
   sucky otherwise... Both conceptually and practically.


I plan to further investigate taint mode execution on IPC::Run/ActiveState, then
possibly falling back on 3. While the Graphviz package might be a cool solution,
it's not (directly) going to solve this problem, so I'd rather have it worked on
a separate patch. Any opinions or suggestions are much welcomed.
Comment 5 Bradley Baetz (:bbaetz) 2003-12-21 00:04:50 PST
GraphViz uses IPC::Run, but newer graphviz versions have a library API, so in
theory it could be rewritten to use XS....
Comment 6 Gervase Markham [:gerv] 2004-01-25 04:20:10 PST
I suggest we go with Jouni's option 3) as the easiest way to get this working on
Win32 in the short term.

Gerv
Comment 7 John M 2004-02-13 10:55:05 PST
[snip]
> 3) Undo the security fix made in bug 197153 for Win32. This is a trivial fix 
>    and will make it work with minimal effort. It shouldn't be a security issue
>    as there is no concept of symlinks on Windows platforms. 
[snip]

Windows 2000's NTFS does support symlinks. It calls them junctions and makes
them a pain to create. See http://support.microsoft.com/?kbid=205524 or
http://www.sysinternals.com/ntw2k/source/misc.shtml#junction .
Comment 8 Byron Jones ‹:glob› 2004-05-19 21:02:38 PDT
> Windows 2000's NTFS does support symlinks. It calls them junctions and makes
> them a pain to create. See http://support.microsoft.com/?kbid=205524 or
> http://www.sysinternals.com/ntw2k/source/misc.shtml#junction .

junctions are for directories only, so bypassing bug 197153 for win32 is ok.
Comment 9 Jouni Heikniemi 2004-05-20 14:22:10 PDT
See <http://aspn.activestate.com/ASPN/docs/ActivePerl/lib/Pod/perlfork.html> for
some new Perl 5.8 related information. I'll try to create a emulated-fork-open
patch for this in the near future. If it works, we'll avoid undoing the security
stuff for Windows (not that it would be an issue with security, but it'd make
the code pretty much unreadable).
Comment 10 Byron Jones ‹:glob› 2004-05-27 05:37:06 PDT
how about something like this:

    # First, generate the png image file from the .dot source

    my ($pngfh, $pngfilename) = File::Temp::tempfile("XXXXXXXXXX",
                                                     SUFFIX => '.png',
                                                     DIR => $webdotdir);
    binmode $pngfh;
    open(DOT, "$webdotbase -Tpng $filename|");
    binmode DOT;
    print $pngfh $_ while <DOT>;
    close $pngfh;
    $vars->{'image_url'} = $pngfilename;

    # Then, generate a imagemap datafile that contains the corner data
    # for drawn bug objects. Pass it on to CreateImagemap that
    # turns this monster into html.
    my ($mapfh, $mapfilename) = File::Temp::tempfile("XXXXXXXXXX",
                                                     SUFFIX => '.map',
                                                     DIR => $webdotdir);
    binmode $mapfh;
    open(DOT, "$webdotbase -Tismap $filename|");
    binmode DOT;
    print $mapfh $_ while <DOT>;
    close $mapfh;
    $vars->{'image_map'} = CreateImagemap($mapfilename);
Comment 11 Byron Jones ‹:glob› 2004-05-28 08:48:26 PDT
Created attachment 149504 [details] [diff] [review]
remove open-pipe from showdependencygraph.cgi

previous comment as a patch
Comment 12 Byron Jones ‹:glob› 2004-05-28 08:49:48 PDT
Comment on attachment 149504 [details] [diff] [review]
remove open-pipe from showdependencygraph.cgi

d'oh.  forgot to close DOT
Comment 13 Byron Jones ‹:glob› 2004-05-28 08:51:14 PDT
Created attachment 149505 [details] [diff] [review]
remove open-pipe from showdependencygraph.cgi v2
Comment 14 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-05-29 10:51:33 PDT
Comment on attachment 149505 [details] [diff] [review]
remove open-pipe from showdependencygraph.cgi v2

>-    open (DOT, '-|') or exec ($webdotbase, "-Tpng", $filename);
>+    open(DOT, "$webdotbase -Tpng $filename|");

>-    open (DOT, '-|') or exec ($webdotbase, "-Tismap", $filename);
>+    open(DOT, "$webdotbase -Tismap $filename|");

Can you absolutely guarantee under all circumstances that $filename will never
contain any shell metacharacters?  This is the reason this construct was
changed to use the fork/exec in the first place.
Comment 15 Jouni Heikniemi 2004-05-30 03:44:43 PDT
I need to examine Byron's approach closer. The question is pretty much whether
or not File::Temp's temporary filenames are shell-safe. They could be; perhaps
changing to pipe open in bug 197153 was unnecessary. The real security issue was
never shell metacharacter interpolation, but rather symlink vulnerability caused
by our use of process ids as tempfilenames. 

As for the emulated fork patch, I tried it. It could work in theory, but I was
unable to turn it into anything reliable enough. I had all sorts of nasty issues
with MySQL DB handles thread cloning not working properly, causing deadlocks and
other not-so-easy-to-debug stuff. Since our external executions are pretty few
at the moment, I'm not interested in wandering further down the open fork path
ATM. If ActiveState starts supporting fork opens, let's use them; meanwhile, I
think we can work around every single issue with relative ease.
Comment 16 Byron Jones ‹:glob› 2004-05-30 18:35:48 PDT
(In reply to comment #15)
> The question is pretty much whether or not File::Temp's temporary filenames
> are shell-safe.

they are, based on the template we are providing.. File::Temp uses

  $path =~ s/X(?=X*\z)/$CHARS[ int( rand( $#CHARS ) ) ]/ge;

to create the temp filename, where

my @CHARS = (qw/ A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
	         a b c d e f g h i j k l m n o p q r s t u v w x y z
	         0 1 2 3 4 5 6 7 8 9 _
	     /);


also, i noticed the original code doesn't set binmode on either filehandles. 
this could cause corrupt images.


on another note, how does this block bug 129401 ?

Comment 17 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-05-31 18:31:59 PDT
I think the dependency was backwards.  Bug 129401 was probably blocking this
one.  Although the other is a more general problem, so breaking the depend
instead of fixing it.
Comment 18 Jouni Heikniemi 2004-06-12 22:46:10 PDT
Comment on attachment 149505 [details] [diff] [review]
remove open-pipe from showdependencygraph.cgi v2

>     my ($pngfh, $pngfilename) = File::Temp::tempfile("XXXXXXXXXX",
>                                                      SUFFIX => '.png',
>                                                      DIR => $webdotdir);
>-    open (DOT, '-|') or exec ($webdotbase, "-Tpng", $filename);
>+    binmode $pngfh;
>+    open(DOT, "$webdotbase -Tpng $filename|");
>+    binmode DOT;

For me, this doesn't work quite right. However, the execution is fine and
that's the most important part anyway. This creates HTML with backslashes in
the img src uri; replacing those with /:s seems to help, though. Did it work on
you, glob? Which browser? I tried both Firefox 0.8 and IE6, and neither
accepted the backslashes (I don't think they should either).
Comment 19 Byron Jones ‹:glob› 2004-06-14 05:45:17 PDT
> This creates HTML with backslashes in the img src uri;
> Did it work on you, glob?

yeah, but as it pans out the http server i'm using is doing the / translation
for me.

sorry about that, new patch coming up.
Comment 20 Byron Jones ‹:glob› 2004-06-14 05:48:32 PDT
Created attachment 150711 [details] [diff] [review]
remove open-pipe from showdependencygraph.cgi v3
Comment 21 Jouni Heikniemi 2004-06-20 00:15:27 PDT
Comment on attachment 150711 [details] [diff] [review]
remove open-pipe from showdependencygraph.cgi v3

r=jouni; tested on XPP/Apache/ASPerl 5.8.3 and Gentoo/Apache/Perl 5.8.1.
Comment 22 Byron Jones ‹:glob› 2004-06-22 00:12:19 PDT
yay!  can someone please check this one in for me.
Comment 23 Vlad Dascalu 2004-06-22 01:06:04 PDT
Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <-- 
showdependencygraph.cgi
new revision: 1.35; previous revision: 1.34
done

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