Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Devel-Cover CPAN distribution.

Report information
The Basics
Id: 49916
Status: resolved
Priority: 0/
Queue: Devel-Cover

People
Owner: Nobody in particular
Requestors: PIOTO [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in:
  • 0.59
  • 0.64
  • 0.65
Fixed in: (no value)

Attachments
0001-Add-loose_perms-option-to-Devel-Cover.patch



Subject: Devel::Cover doesn't like code that changes EUID
Download (untitled) / with headers
text/plain 496b
I was looking to do some coverage on code which does part of its work by forking and changing its EUID. However, the restrictive permissions which are placed on the cover_db directory and things under it seem to cause problems. t/run_as_user.....ok 1/11Can't mkdir /home/mike/foo/cover_db/runs: Permission denied at t/run_as_user.t line 0 I'm not sure if there's a straightforward solution to this or not. If it would help helpful, I can try to create a reduced test case to demonstrate this.
Download (untitled) / with headers
text/plain 258b
Hello, Producing a test case would be helpful, but I wonder whether the easiest and possibly the best solution would be to specify the location of the directory somewhere where your program would have permissions to write to it? Would that work for you?
Download (untitled) / with headers
text/plain 511b
On Sun Apr 11 12:49:19 2010, PJCJ wrote: Show quoted text
> Hello, > > Producing a test case would be helpful, but I wonder whether the easiest > and possibly the best solution would be to specify the location of the > directory somewhere where your program would have permissions to write to > it? Would that work for you?
Yeah, that would mostly cover it, except for the chmods that are in there with the mkdirs. If I could just specify some other path, and have it not try to mess with the permissions, that would work.
Download (untitled) / with headers
text/plain 360b
I've removed the chmods which should hopefully fix your problem. They were only in there to fix a problem with perl5.6.x which I don't seem to be able to reproduce any more anyway, so we'll see if any 5.6.x uses shout. If set the status to resolved and this will be in the next release, but please reopen the ticket if there are still problems. Thanks,
Download (untitled) / with headers
text/plain 1.2k
On Mon Apr 12 06:34:09 2010, PJCJ wrote: Show quoted text
> I've removed the chmods which should hopefully fix your problem. They > were only in there to fix a problem with perl5.6.x which I don't seem > to be able to reproduce any more anyway, so we'll see if any 5.6.x > uses shout. > > If set the status to resolved and this will be in the next release, > but please reopen the ticket if there are still problems.
If this commit is the purported fix: http://github.com/pjcj/Devel--Cover/commit/8b26a6ffee8be0c33f492d715feef5bf2ae09aba I think you may have misunderstood me. The issue isn't the chmod() specifically, it's the fact that you're specifying such a restrictive mask on the mkdir, too. This means that, when my child processes start to do stuff as non-root, they won't be able to write to the coverage directory. The mkdir pod suggests that you'd be better off not specifying the mask at all: In general, it is better to create directories with permissive MASK, and let the user modify that with their "umask", than it is to supply a restrictive MASK and give the user no way to be more permissive. I don't really think there's anything inherently private about the coverage database, though I could be wrong. If not, then I think that simply removing the mask as well should help address my issue. Thanks.
Download (untitled) / with headers
text/plain 908b
I have solved this issue with a monkey patch :) But I double the request to make the umask in mkdir configurable from outside or at least remove it at all. Here is my startup.pl with a monkey patch for mod_perl2 #!/usr/bin/perl use strict; use warnings; use Apache2::Directive (); BEGIN { # Devel::Cover database must be writable by worker processes my $conftree = Apache2::Directive::conftree->as_hash; my $name = $conftree->{User} or die "couldn't find user in Apache config"; print "user=$name\n"; my $uid = getpwnam($name); defined $uid or die "couldn't determine uid by name"; no warnings 'redefine'; local $> = $uid; require Devel::Cover; my $old_report = \&Devel::Cover::report; *Devel::Cover::report = sub { local $> = $uid; $old_report->(@_) }; Devel::Cover->import; } 1;
The patch that we're using to address this bug is attached.
Subject: 0001-Add-loose_perms-option-to-Devel-Cover.patch
From aaeeafb5f7c8d72f275fcb8f7747e0b6a102f315 Mon Sep 17 00:00:00 2001 From: John Lightsey <jd@cpanel.net> Date: Thu, 22 Sep 2011 10:27:01 -0500 Subject: [PATCH] Add -loose_perms option to Devel::Cover. RT 49916: Test code that changes EUID before exiting cannot write to the cover db directory by default due to restrictive file/directory permissions. This is corrected with an option to give all cover DB directories 777 permisisons. --- lib/Devel/Cover.pm | 14 +++++++++++++- lib/Devel/Cover/DB.pm | 3 +++ lib/Devel/Cover/DB/IO/JSON.pm | 1 + lib/Devel/Cover/DB/IO/Storable.pm | 1 + lib/Devel/Cover/DB/Structure.pm | 3 +++ 5 files changed, 21 insertions(+), 1 deletions(-) diff --git a/lib/Devel/Cover.pm b/lib/Devel/Cover.pm index ab56f18..6a94a49 100755 --- a/lib/Devel/Cover.pm +++ b/lib/Devel/Cover.pm @@ -54,6 +54,7 @@ my $Summary = 1; # Output coverage summary. my $Subs_only = 0; # Coverage only for sub bodies. my $Self_cover; # Coverage of Devel::Cover. my $Self_cover_run = 0; # Covering Devel::Cover now. +my $Loose_perms = 0; # Use loose permissions in the cover DB my @Ignore; # Packages to ignore. my @Inc; # Original @INC to ignore. @@ -284,6 +285,7 @@ sub import /^-silent/ && do { $Silent = shift @o; next }; /^-dir/ && do { $Dir = shift @o; next }; /^-db/ && do { $DB = shift @o; next }; + /^-loose_perms/ && do { $Loose_perms = shift @o; next }; /^-merge/ && do { $Merge = shift @o; next }; /^-summary/ && do { $Summary = shift @o; next }; /^-blib/ && do { $blib = shift @o; next }; @@ -331,6 +333,9 @@ sub import { mkdir $DB, 0700 or die "Can't mkdir $DB: $!"; } + if ($Loose_perms) { + chmod 0777, $DB; + } $DB = $1 if abs_path($DB) =~ /(.*)/; Devel::Cover::DB->delete($DB) unless $Merge; @@ -665,7 +670,7 @@ sub _report chdir $Dir or die __PACKAGE__ . ": Can't chdir $Dir: $!\n"; $Run{collected} = \@collected; - $Structure = Devel::Cover::DB::Structure->new(base => $DB); + $Structure = Devel::Cover::DB::Structure->new(base => $DB, loose_perms => $Loose_perms); $Structure->read_all; $Structure->add_criteria(@collected); # print STDERR "Start structure: ", Dumper $Structure; @@ -725,6 +730,7 @@ sub _report base => $DB, runs => { $run => \%Run }, structure => $Structure, + loose_perms => $Loose_perms, ); my $dbrun = "$DB/runs"; @@ -732,6 +738,9 @@ sub _report { mkdir $dbrun, 0700 or croak "Can't mkdir $dbrun: $!\n"; } + if ($Loose_perms) { + chmod 0777, $dbrun; + } $dbrun .= "/$run"; print OUT __PACKAGE__, ": Writing coverage database to $dbrun\n" @@ -1378,6 +1387,9 @@ if the tests fail and you would like nice output telling you why. +ignore RE - Append to REs of files to ignore. -inc path - Set prefixes of files to ignore (default @INC). +inc path - Append to prefixes of files to ignore. + -loose_perms val - Use loose permissions on all files and directories in + the coverage db so that code changing EUID can still + write coverage information (default off). -merge val - Merge databases, for multiple test benches (default on). -select RE - Set REs of files to select (default none). +select RE - Append to REs of files to select. diff --git a/lib/Devel/Cover/DB.pm b/lib/Devel/Cover/DB.pm index 3fed6ac..8a5f397 100644 --- a/lib/Devel/Cover/DB.pm +++ b/lib/Devel/Cover/DB.pm @@ -83,6 +83,9 @@ sub write { mkdir $self->{db}, 0700 or croak "Can't mkdir $self->{db}: $!\n"; } + if ($self->{loose_perms}) { + chmod 0777, $self->{db}; + } $self->validate_db; my $db = { runs => $self->{runs} }; diff --git a/lib/Devel/Cover/DB/IO/JSON.pm b/lib/Devel/Cover/DB/IO/JSON.pm index f1d305d..a6a26e1 100644 --- a/lib/Devel/Cover/DB/IO/JSON.pm +++ b/lib/Devel/Cover/DB/IO/JSON.pm @@ -42,6 +42,7 @@ sub write my $json = JSON::PP->new->utf8; $json->ascii->pretty->canonical if $self->{options} =~ /\bpretty\b/i; + unlink $file; open my $fh, ">", $file or die "Can't open $file: $!"; flock($fh, LOCK_EX) or die "Cannot lock file: $!\n"; print $fh $json->encode($data); diff --git a/lib/Devel/Cover/DB/IO/Storable.pm b/lib/Devel/Cover/DB/IO/Storable.pm index 0608b7e..a24b97e 100644 --- a/lib/Devel/Cover/DB/IO/Storable.pm +++ b/lib/Devel/Cover/DB/IO/Storable.pm @@ -34,6 +34,7 @@ sub write my $self = shift; my ($data, $file) = @_; + unlink $file; Storable::lock_nstore($data, $file); $self } diff --git a/lib/Devel/Cover/DB/Structure.pm b/lib/Devel/Cover/DB/Structure.pm index d7dc13c..b128285 100644 --- a/lib/Devel/Cover/DB/Structure.pm +++ b/lib/Devel/Cover/DB/Structure.pm @@ -274,6 +274,9 @@ sub write { mkdir $dir, 0700 or croak "Can't mkdir $dir: $!\n"; } + if ($self->{loose_perms}) { + chmod 0777, $dir; + } for my $file (sort keys %{$self->{f}}) { $self->{f}{$file}{file} = $file; -- 1.7.5
This patch works well for my needs too.
Download (untitled) / with headers
text/plain 423b
Is this still a problem? A couple of months ago I made the change in https://github.com/pjcj/Devel--Cover/commit/97a6340 which removed the explicit 0700 directory permissions. Does anything else need to be done? John's patch contains a couple of unlinks. I imagine they're not necessary anymore. Or are they? If anyone has a test case for this, or at least a set of instructions to reproduce it, I'd be grateful.
Download (untitled) / with headers
text/plain 175b
In the absence of any further information I'm going to assume that this is now fixed and close the ticket. If there is still a problem, please feel free to reopen the ticket.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.