From: dklawren <dklawren@users.noreply.github.com>
Date: Mon, 4 May 2020 17:37:43 +0000 (-0400)
Subject: Bug 1635148 - Recent changes to add support for attachments in S3 caused a memory... 
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=77cc3da808b33da1005c8d6d8913b80a378a9c9c;p=thirdparty%2Fbugzilla.git

Bug 1635148 - Recent changes to add support for attachments in S3 caused a memory leak in the phabbugz feed daemon
---

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index f82808d58..12d7bd267 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -936,15 +936,15 @@ sub get_storage_by_name {
   # all options for attachment_storage need to be handled here
   if ($name eq 'database') {
     require Bugzilla::Attachment::Storage::Database;
-    return Bugzilla::Attachment::Storage::Database->new({attachment => $self});
+    return Bugzilla::Attachment::Storage::Database->new({attach_id => $self->id});
   }
   elsif ($name eq 'filesystem') {
     require Bugzilla::Attachment::Storage::FileSystem;
-    return Bugzilla::Attachment::Storage::FileSystem->new({attachment => $self});
+    return Bugzilla::Attachment::Storage::FileSystem->new({attach_id => $self->id});
   }
   elsif ($name eq 's3') {
     require Bugzilla::Attachment::Storage::S3;
-    return Bugzilla::Attachment::Storage::S3->new({attachment => $self});
+    return Bugzilla::Attachment::Storage::S3->new({attach_id => $self->id, datasize => $self->datasize});
   }
   else {
     return undef;
diff --git a/Bugzilla/Attachment/Storage/Base.pm b/Bugzilla/Attachment/Storage/Base.pm
index 4725f05c7..780aa2da2 100644
--- a/Bugzilla/Attachment/Storage/Base.pm
+++ b/Bugzilla/Attachment/Storage/Base.pm
@@ -10,28 +10,28 @@ package Bugzilla::Attachment::Storage::Base;
 use 5.10.1;
 use Moo::Role;
 
-use Type::Utils qw(class_type);
+use Types::Standard qw(Int);
 
 requires qw(set_data get_data remove_data data_exists data_type);
 
-has 'attachment' => (
+has 'attach_id' => (
   is       => 'ro',
   required => 1,
-  isa      => class_type({class => 'Bugzilla::Attachment'})
+  isa      => Int
 );
 
 sub set_class {
   my ($self) = @_;
   Bugzilla->dbh->do(
     "REPLACE INTO attachment_storage_class (id, storage_class) VALUES (?, ?)",
-    undef, $self->attachment->id, $self->data_type);
+    undef, $self->attach_id, $self->data_type);
   return $self;
 }
 
 sub remove_class {
   my ($self) = @_;
   Bugzilla->dbh->do("DELETE FROM attachment_storage_class WHERE id = ?",
-    undef, $self->attachment->id);
+    undef, $self->attach_id);
   return $self;
 }
 
diff --git a/Bugzilla/Attachment/Storage/Database.pm b/Bugzilla/Attachment/Storage/Database.pm
index 9da52ef6c..22eaf762b 100644
--- a/Bugzilla/Attachment/Storage/Database.pm
+++ b/Bugzilla/Attachment/Storage/Database.pm
@@ -21,7 +21,7 @@ sub set_data {
     = $dbh->prepare(
     "REPLACE INTO attach_data (id, thedata) VALUES (?, ?)"
     );
-  $sth->bind_param(1, $self->attachment->id);
+  $sth->bind_param(1, $self->attach_id);
   $sth->bind_param(2, $data, $dbh->BLOB_TYPE);
   $sth->execute();
   return $self;
@@ -32,14 +32,14 @@ sub get_data {
   my $dbh = Bugzilla->dbh;
   my ($data)
     = $dbh->selectrow_array("SELECT thedata FROM attach_data WHERE id = ?",
-    undef, $self->attachment->id);
+    undef, $self->attach_id);
   return $data;
 }
 
 sub remove_data {
   my ($self) = @_;
   my $dbh = Bugzilla->dbh;
-  $dbh->do("DELETE FROM attach_data WHERE id = ?", undef, $self->attachment->id);
+  $dbh->do("DELETE FROM attach_data WHERE id = ?", undef, $self->attach_id);
   return $self;
 }
 
@@ -47,7 +47,7 @@ sub data_exists {
   my ($self)   = @_;
   my $dbh      = Bugzilla->dbh;
   my ($exists) = $dbh->selectrow_array("SELECT 1 FROM attach_data WHERE id = ?",
-    undef, $self->attachment->id);
+    undef, $self->attach_id);
   return !!$exists;
 }
 
diff --git a/Bugzilla/Attachment/Storage/FileSystem.pm b/Bugzilla/Attachment/Storage/FileSystem.pm
index 7a67d1b3c..50e7f7230 100644
--- a/Bugzilla/Attachment/Storage/FileSystem.pm
+++ b/Bugzilla/Attachment/Storage/FileSystem.pm
@@ -51,13 +51,13 @@ sub data_exists {
 
 sub _local_path {
   my ($self) = @_;
-  my $hash = sprintf 'group.%03d', $self->attachment->id % 1000;
+  my $hash = sprintf 'group.%03d', $self->attach_id % 1000;
   return bz_locations()->{attachdir} . '/' . $hash;
 }
 
 sub _local_file {
   my ($self) = @_;
-  return $self->_local_path() . '/attachment.' . $self->attachment->id;
+  return $self->_local_path() . '/attachment.' . $self->attach_id;
 }
 
 1;
diff --git a/Bugzilla/Attachment/Storage/S3.pm b/Bugzilla/Attachment/Storage/S3.pm
index aea04572c..afb25e452 100644
--- a/Bugzilla/Attachment/Storage/S3.pm
+++ b/Bugzilla/Attachment/Storage/S3.pm
@@ -13,10 +13,17 @@ use Moo;
 use Bugzilla::Error;
 use Bugzilla::S3;
 
+use Types::Standard qw(Int);
+
 with 'Bugzilla::Attachment::Storage::Base';
 
 has 's3'     => (is => 'lazy');
 has 'bucket' => (is => 'lazy');
+has 'datasize' => (
+  is       => 'ro',
+  required => 1,
+  isa      => Int
+);
 
 sub _build_s3 {
   my $self = shift;
@@ -38,15 +45,15 @@ sub data_type { return 's3'; }
 
 sub set_data {
   my ($self, $data) = @_;
-  my $attach_id = $self->attachment->id;
+  my $attach_id = $self->attach_id;
 
   # If the attachment is larger than attachment_s3_minsize,
   # we instead store it in the database.
   if (Bugzilla->params->{attachment_s3_minsize}
-    && $self->attachment->datasize < Bugzilla->params->{attachment_s3_minsize})
+    && $self->datasize < Bugzilla->params->{attachment_s3_minsize})
   {
     require Bugzilla::Attachment::Storage::Database;
-    return Bugzilla::Attachment::Storage::Database->new({attachment => $self->attachment})
+    return Bugzilla::Attachment::Storage::Database->new({attach_id => $self->attach_id})
       ->set_data($data);
   }
 
@@ -62,7 +69,7 @@ sub set_data {
 
 sub get_data {
   my ($self)    = @_;
-  my $attach_id = $self->attachment->id;
+  my $attach_id = $self->attach_id;
   my $response  = $self->bucket->get_key($attach_id);
   if (!$response) {
     warn "Failed to retrieve attachment $attach_id from S3: "
@@ -75,7 +82,7 @@ sub get_data {
 
 sub remove_data {
   my ($self) = @_;
-  my $attach_id = $self->attachment->id;
+  my $attach_id = $self->attach_id;
   $self->bucket->delete_key($attach_id)
     or warn "Failed to remove attachment $attach_id from S3: "
     . $self->bucket->errstr . "\n";
@@ -84,7 +91,7 @@ sub remove_data {
 
 sub data_exists {
   my ($self) = @_;
-  return !!$self->bucket->head_key($self->attachment->id);
+  return !!$self->bucket->head_key($self->attach_id);
 }
 
 1;
diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index dcb5e020e..baa887677 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -384,7 +384,7 @@ sub process_revision_change {
   );
   INFO($log_message);
 
-# change to the phabricator user, which returns a guard that restores the previous user.
+  # change to the phabricator user, which returns a guard that restores the previous user.
   my $restore_prev_user = set_phab_user();
   my $bug               = $revision->bug;
 
@@ -460,7 +460,7 @@ sub process_revision_change {
 
   INFO('Checking for revision attachment');
   my $rev_attachment = create_revision_attachment($bug, $revision, $timestamp,
-    $revision->author->bugzilla_user);
+  $revision->author->bugzilla_user);
   INFO('Attachment ' . $rev_attachment->id . ' created or already exists.');
 
   # ATTACHMENT OBSOLETES
@@ -630,7 +630,6 @@ sub process_new_user {
     f7 => 'reporter',
     o7 => 'equals',
     v7 => $bug_user->login,
-
     f9 => 'CP',
 
     # The bug needs to be private