Closed
Bug 250976
Opened 20 years ago
Closed 15 years ago
need a standard way to call piped open
Categories
(Bugzilla :: Bugzilla-General, defect)
Bugzilla
Bugzilla-General
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: glob, Unassigned)
Details
Attachments
(1 file, 5 obsolete files)
7.98 KB,
patch
|
jouni
:
review-
|
Details | Diff | Splinter Review |
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.
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?
this revision adds ThrowCodeError and fixes a bug in pipe_open where errors were still returning a filehandle
Attachment #152917 -
Attachment is obsolete: true
oops, typo
Attachment #153006 -
Attachment is obsolete: true
Attachment #153007 -
Flags: review?
Comment 5•20 years ago
|
||
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-
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 :)
Attachment #153007 -
Attachment is obsolete: true
Attachment #153036 -
Flags: review?(gerv)
Comment 7•20 years ago
|
||
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+
previous patch does "my $dotfh" twice in showdependencygraph, raising a warning.
Attachment #153036 -
Attachment is obsolete: true
Attachment #153775 -
Flags: review?(bbaetz)
Comment 10•20 years ago
|
||
Comment on attachment 153036 [details] [diff] [review] centralise pipe open calls v4 Removing r? from obsolete attachment.
Attachment #153036 -
Flags: review?(bbaetz)
Updated•20 years ago
|
Whiteboard: patch awaitng review
Reporter | ||
Comment 11•19 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)
Attachment #153775 -
Flags: review?(jouni)
Attachment #153775 -
Attachment is obsolete: true
Attachment #153775 -
Flags: review?(jouni)
Reporter | ||
Comment 12•19 years ago
|
||
bitrot fixes.
Attachment #180684 -
Flags: review?(jouni)
Comment 13•19 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•19 years ago
|
Whiteboard: patch awaitng review
Updated•18 years ago
|
QA Contact: mattyt-bugzilla → default-qa
Comment 14•15 years ago
|
||
We don't use piped opens anymore, except perhaps for dependency graphs.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•