225359 - Drawing dependency graphs is broken on Win32

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Jouni Heikniemi]

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]

Oh and Bradley, if you want to take this, I don't mind at all. :-)

Comment 2

[Jouni Heikniemi]

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)]

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]

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)]

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]]

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]

[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

[:glob ✱]

> 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]

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

[:glob ✱]

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

[:glob ✱]

Attachment: remove open-pipe from showdependencygraph.cgi

previous comment as a patch

Comment 12

[:glob ✱]

Comment on attachment 149504 [details] [diff] [review]
remove open-pipe from showdependencygraph.cgi

d'oh.  forgot to close DOT

Comment 13

[:glob ✱]

Attachment: remove open-pipe from showdependencygraph.cgi v2

Comment 14

[Dave Miller [:justdave]]

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]

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

[:glob ✱]

(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]]

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]

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

[:glob ✱]

> 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

[:glob ✱]

Attachment: remove open-pipe from showdependencygraph.cgi v3

Comment 21

[Jouni Heikniemi]

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

[:glob ✱]

yay!  can someone please check this one in for me.

Comment 23

[Vlad Dascalu]

Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <-- 
showdependencygraph.cgi
new revision: 1.35; previous revision: 1.34
done