Bug 1236161 - when converting a BMP attachment to PNG fails a zero byte attachment is created

2 files changed, 98 insertions, 7 deletions

diff --git a/extensions/BmpConvert/Extension.pm b/extensions/BmpConvert/Extension.pm
index ee08e04f0..efffa91b2 100644
--- a/extensions/BmpConvert/Extension.pm
+++ b/extensions/BmpConvert/Extension.pm
@@ -25,33 +25,82 @@ use strict;
 use base qw(Bugzilla::Extension);
 
 use Image::Magick;
+use File::Temp qw(:seekable);
+use File::stat qw(stat);
 
 our $VERSION = '1.0';
 
 sub attachment_process_data {
     my ($self, $args) = @_;
+
     return unless $args->{attributes}->{mimetype} eq 'image/bmp';
+    eval {
+        _try_convert_bmp_to_png($args);
+    };
+    warn $@ if $@;
+}
+
+
+# Here be dragons:
+# Image::Magick uses dualvars extensively to signal errors.
+# The documentation is either confusing or wrong in this regard.
+# This is not a great practice. dualvar(0, "foo") is a true value,
+# but dualvar(0, "foo") + 0 is not.
+# Also dualvar(1, "") is a false value, but dualvar(1, "") > 0 is true.
+#
+# "When a scalar has both string and numeric components (dualvars), Perl
+# prefers to check the string component for boolean truth."
+# From https://github.com/chromatic/modern_perl_book/blob/master/sections/coercion.pod
+sub _try_convert_bmp_to_png {
+    my ($args) = @_;
 
     my $data = ${$args->{data}};
     my $img = Image::Magick->new(magick => 'bmp');
+    my $size;
 
-    # $data is a filehandle.
     if (ref $data) {
-        $img->Read(file => \*$data);
+        my $read_error = $img->Read(file => \*$data);
+
+        # rewind so it can be read in again by other code
+        seek($data, 0, SEEK_SET);
+
+        die "Error reading in BMP: $read_error"
+          if $read_error;
+
         $img->set(magick => 'png');
-        $img->Write(file => \*$data);
+
+        my $tmp = File::Temp->new(UNLINK => 1, SUFFIX => '.png');
+        my $write_error = $img->Write(file => $tmp);
+
+        die "Error converting BMP to PNG: $write_error"
+          if $write_error;
+
+        $tmp->flush;
+        $size = stat($tmp->filename)->size;
+        die "Error converting BMP to PNG results in empty file"
+          if $size == 0;
+
+        $tmp->seek(0, SEEK_SET);
+        $data = $tmp;
     }
-    # $data is a blob.
     else {
-        $img->BlobToImage($data);
+        my $parse_error = $img->BlobToImage($data);
+        die "Error parsing in BMP: $parse_error"
+          if $parse_error;
+
         $img->set(magick => 'png');
         $data = $img->ImageToBlob();
+
+        die "Error converting BMP to PNG (empty PNG)"
+          unless length($data) > 0;
+
+        $size = length($data);
     }
-    undef $img;
 
     ${$args->{data}} = $data;
     $args->{attributes}->{mimetype} = 'image/png';
-    $args->{attributes}->{filename} =~ s/^(.+)\.bmp$/$1.png/i;
+    $args->{attributes}->{filename} =~ s/\.bmp$/.png/i;
+    $args->{attributes}->{attach_size} = $size;
 }
 
  __PACKAGE__->NAME;
diff --git a/scripts/fix-attachment-sizes.pl b/scripts/fix-attachment-sizes.pl
new file mode 100644
index 000000000..a50e241ce
--- /dev/null
+++ b/scripts/fix-attachment-sizes.pl
@@ -0,0 +1,42 @@
+#!/usr/bin/perl -w
+
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+
+use strict;
+use feature 'say';
+
+use FindBin qw( $RealBin );
+use lib "$RealBin/..";
+use lib "$RealBin/../lib";
+
+use Bugzilla;
+use Bugzilla::Constants;
+
+Bugzilla->usage_mode(USAGE_MODE_CMDLINE);
+
+my $dbh = Bugzilla->dbh;
+
+$dbh->bz_start_transaction();
+my $attachment_sizes = $dbh->selectall_arrayref(q{
+        SELECT attachments.attach_id, length(thedata)
+        FROM attach_data
+        INNER JOIN attachments ON attachments.attach_id = attach_data.id
+        WHERE attachments.attach_size != 0
+          AND attachments.mimetype = 'image/png'
+          AND length(thedata) != attachments.attach_size });
+say "Found ", scalar @$attachment_sizes, " attachments to fix";
+
+foreach my $attachment_size (@$attachment_sizes) {
+    say "Setting size for $attachment_size->[0] to $attachment_size->[1]";
+
+    $dbh->do("UPDATE attachments SET attach_size = ? WHERE attach_id = ?", undef,
+             $attachment_size->[1],
+             $attachment_size->[0]);
+}
+
+$dbh->bz_commit_transaction();
\ No newline at end of file