need a standard way to call piped open

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
15 years ago
9 years ago

People

(Reporter: glob, Unassigned)

Tracking

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Created attachment 152917 [details] [diff] [review]
centralise pipe open calls
(Reporter)

Updated

15 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 2

15 years ago
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?
(Reporter)

Comment 3

15 years ago
Created attachment 153006 [details] [diff] [review]
centralise pipe open calls v2

this revision adds ThrowCodeError and fixes a bug in pipe_open where errors
were still returning a filehandle
Attachment #152917 - Attachment is obsolete: true
(Reporter)

Comment 4

15 years ago
Created attachment 153007 [details] [diff] [review]
centralise pipe open calls v3

oops, typo
Attachment #153006 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #153007 - Flags: review?
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
Attachment #153007 - Flags: review? → review-
(Reporter)

Comment 6

15 years ago
Created attachment 153036 [details] [diff] [review]
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 :)
(Reporter)

Updated

15 years ago
Attachment #153007 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #153036 - Flags: review?(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
Attachment #153036 - Flags: review?(gerv)
Attachment #153036 - Flags: review?(bbaetz)
Attachment #153036 - Flags: review+
(Reporter)

Comment 8

14 years ago
Created attachment 153775 [details] [diff] [review]
centralise pipe open calls v5

previous patch does "my $dotfh" twice in showdependencygraph, raising a
warning.
Attachment #153036 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
Attachment #153775 - Flags: review?(bbaetz)
(Reporter)

Comment 9

14 years ago
bbaetz: ping
Comment on attachment 153036 [details] [diff] [review]
centralise pipe open calls v4

Removing r? from obsolete attachment.
Attachment #153036 - Flags: review?(bbaetz)

Updated

14 years ago
Whiteboard: patch awaitng review
(Reporter)

Comment 11

14 years ago
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.
Attachment #153775 - Flags: review?(bbaetz)
(Reporter)

Updated

14 years ago
Attachment #153775 - Flags: review?(jouni)
(Reporter)

Updated

14 years ago
Attachment #153775 - Attachment is obsolete: true
Attachment #153775 - Flags: review?(jouni)
(Reporter)

Comment 12

14 years ago
Created attachment 180684 [details] [diff] [review]
centralise pipe open calls v6

bitrot fixes.
(Reporter)

Updated

14 years ago
Attachment #180684 - Flags: review?(jouni)

Comment 13

14 years ago
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...
Attachment #180684 - Flags: review?(jouni) → review-

Updated

14 years ago
Whiteboard: patch awaitng review

Updated

13 years ago
QA Contact: mattyt-bugzilla → default-qa
(Reporter)

Updated

12 years ago
Assignee: bugzilla → general
Status: ASSIGNED → NEW

Comment 14

9 years ago
We don't use piped opens anymore, except perhaps for dependency graphs.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.