250976 - need a standard way to call piped open

Status: RESOLVED WONTFIX

(Bugzilla :: Bugzilla-General, defect)

Description

[:glob ✱]

we should use a standard call to run piped opens, as a wrapper in Bugzilla::Util.

so the fixes to get piped open working on windows will be in one location, and
we can also add the perl 5.8+ specific call to catch errors during opening the pipe.

Comment 1

[:glob ✱]

Attachment: centralise pipe open calls

Comment 2

[:glob ✱]

this should fix issues where there are spaces in the temp directory
(attachment.cgi interdiff) or in the bugzilla install path
(showdependencygraph.cgi webdot).

it also allows the developer to catch errors during the exec, which doesn't
happen at all right now.  i'm not sure if these should be considered code or
user errors however, so there's dies with XXX's right now.

comments?

Comment 3

[:glob ✱]

Attachment: centralise pipe open calls v2

this revision adds ThrowCodeError and fixes a bug in pipe_open where errors
were still returning a filehandle

Comment 4

[:glob ✱]

Attachment: centralise pipe open calls v3

oops, typo

Comment 5

[Gervase Markham [:gerv]]

Comment on attachment 153007 [details] [diff] [review]
centralise pipe open calls v3

You should not print out the full pathname of the program and the error it
gives in the error message given back to the user; that would reveal too much
information useful to an attacker.

You should add a test to the test suite which checks for calls to a piped open
in all other files and flags them as an error. This might be easier practically
if the function was called e.g. open_piped() instead of pipe_open().

Does File::Basename ship by default with all versions of Perl we currently
support?

What's the comment at the top of the pipe_open function for?

Gerv

Comment 6

[:glob ✱]

Attachment: centralise pipe open calls v4

updated patch..

> You should not print out the full pathname of the program and the error it
> gives in the error message given back to the user; that would reveal too much

> information useful to an attacker.

i'm not, i'm just printing out the basename.

> You should add a test to the test suite which checks for calls to a piped
> open in all other files and flags them as an error. 

i have that in the works already, and i was planning to put that in another
bug.

so, instead, here it is.  it also catches calls to fork, so i've called it
cross_platform.t
note that i had to fix an error message in collectstats.pl that incorrectly
referred to fork.

> This might be easier practically if the function was called 
> e.g. open_piped() instead of pipe_open().

done

> Does File::Basename ship by default with all versions of Perl we currently
> support?

it started shipping with perl 5.6, which is the minimum perl version for 2.18+

> What's the comment at the top of the pipe_open function for?

oops, gone :)

Comment 7

[Gervase Markham [:gerv]]

Comment on attachment 153036 [details] [diff] [review]
centralise pipe open calls v4

OK, r=gerv, but I want signoff from a security person on the errors.

Gerv

Comment 8

[:glob ✱]

Attachment: centralise pipe open calls v5

previous patch does "my $dotfh" twice in showdependencygraph, raising a
warning.

Comment 9

[:glob ✱]

bbaetz: ping

Comment 10

[Marc Schumann [:Wurblzap]]

Comment on attachment 153036 [details] [diff] [review]
centralise pipe open calls v4

Removing r? from obsolete attachment.

Comment 11

[:glob ✱]

Comment on attachment 153775 [details] [diff] [review]
centralise pipe open calls v5

gerv -- no response from bbaetz after a looooong time and a few reminders (both
on this bug and irc).

if you want someone else to review this bug, please request someone active.

for now, i'm throwing this one "to the wind" again.

Comment 12

[:glob ✱]

Attachment: centralise pipe open calls v6

bitrot fixes.

Comment 13

[Jouni Heikniemi]

Comment on attachment 180684 [details] [diff] [review]
centralise pipe open calls v6

>Index: t/010crossplatform.t
>===================================================================

This won't work now; test #10 is already taken. Make this #11.

>+my @testitems = @Support::Files::testitems;
>+for my $path (@Support::Templates::include_paths) {
>+   push(@testitems, map(File::Spec->catfile($path, $_),
>+                        Support::Templates::find_actual_files($path)));
>+}

Add a comment before this block to explain what we're including here.

>+        if (/\bopen\b[^'"]+['"]-\|['"]/) {

Hmm... This doesn't actually work, you know. It doesn't match even close to all
of the instances of pipe opens you're renaming.

>+        } elsif (/\bfork\b/) {
>+            push @errors, "contains fork at line $.";
>+        }

Why do I get the feeling this isn't such a good idea. "fork" is actually a
rather useful word in comments and so on...

Since neither of the tests you're trying to do are trivial to get working
right, I can't help but feeling we should forget about this test.

>Index: collectstats.pl
>===================================================================
>     # Generate a static RDF file containing the default view of the duplicates data.
>     open(CGI, "$perl -T duplicates.cgi |")
>-        || die "can't fork duplicates.cgi: $!";
>+        || die "can't execute duplicates.cgi: $!";

So why are we leaving this pipe open here?

>Index: template/en/default/global/code-error.html.tmpl
>===================================================================
>+  [% ELSIF error == "execute_failed" %]
>+    [% title = "Execution Failed" %]
>+    Failed to execute external program <em>[% program FILTER html %]</em>.<br>
>+    <br>
>+    [% error_msg FILTER html %]

I don't think we should be leaking the program name in the error message...

Comment 14

[Max Kanat-Alexander]

We don't use piped opens anymore, except perhaps for dependency graphs.