Closed
Bug 225359
Opened 21 years ago
Closed 20 years ago
Drawing dependency graphs is broken on Win32
Categories
(Bugzilla :: Reporting/Charting, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: jouni, Assigned: glob)
Details
(Whiteboard: [needed for Win32bz])
Attachments
(1 file, 2 obsolete files)
1.40 KB,
patch
|
jouni
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•21 years ago
|
||
Oh and Bradley, if you want to take this, I don't mind at all. :-)
Summary: Drawing dependency charts is broken on Win32 → Drawing dependency graphs is broken on Win32
Whiteboard: [needed for Win32bz]
Target Milestone: --- → Bugzilla 2.18
Reporter | ||
Comment 2•21 years ago
|
||
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?
Severity: normal → major
Status: NEW → ASSIGNED
Comment 3•21 years ago
|
||
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.
Reporter | ||
Comment 4•21 years ago
|
||
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•21 years ago
|
||
GraphViz uses IPC::Run, but newer graphviz versions have a library API, so in theory it could be rewritten to use XS....
Comment 6•21 years ago
|
||
I suggest we go with Jouni's option 3) as the easiest way to get this working on Win32 in the short term. Gerv
[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 .
Updated•20 years ago
|
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
> 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.
Reporter | ||
Comment 9•20 years ago
|
||
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).
Assignee | ||
Comment 10•20 years ago
|
||
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);
Assignee | ||
Comment 11•20 years ago
|
||
previous comment as a patch
Assignee | ||
Comment 12•20 years ago
|
||
Comment on attachment 149504 [details] [diff] [review] remove open-pipe from showdependencygraph.cgi d'oh. forgot to close DOT
Attachment #149504 -
Attachment is obsolete: true
Assignee | ||
Comment 13•20 years ago
|
||
Attachment #149505 -
Flags: review?
Comment 14•20 years ago
|
||
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.
Reporter | ||
Comment 15•20 years ago
|
||
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.
Assignee | ||
Comment 16•20 years ago
|
||
(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•20 years ago
|
||
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.
No longer blocks: 129401
Reporter | ||
Comment 18•20 years ago
|
||
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).
Attachment #149505 -
Flags: review? → review-
Assignee | ||
Comment 19•20 years ago
|
||
> 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.
Assignee | ||
Comment 20•20 years ago
|
||
Attachment #149505 -
Attachment is obsolete: true
Attachment #150711 -
Flags: review?(jouni)
Reporter | ||
Comment 21•20 years ago
|
||
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.
Attachment #150711 -
Flags: review?(jouni) → review+
Updated•20 years ago
|
Flags: approval? → approval+
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
Assignee | ||
Comment 22•20 years ago
|
||
yay! can someone please check this one in for me.
Comment 23•20 years ago
|
||
Checking in showdependencygraph.cgi; /cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v <-- showdependencygraph.cgi new revision: 1.35; previous revision: 1.34 done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•