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

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[YF (Yang)]

attachment 8703286 [details] of bug 1236152.


https://bugzilla.mozilla.org/attachment.cgi?id=8703286&action=edit says:
Fullscreen capture 01-01-2016 194503.png (image/png), 3.87 MB

But, curl -v https://bug1236152.bmoattachments.org/attachment.cgi?id=8703286

< HTTP/1.1 200 OK
< Date: Fri, 01 Jan 2016 20:08:53 GMT
< Server: Apache
< X-xss-protection: 1; mode=block
< Content-disposition: inline; filename="Fullscreen capture 01-01-2016 194503.png"
< X-content-type-options: nosniff
< Set-Cookie: Bugzilla_login_request_cookie=CAyuWhMyFB; path=/; secure; HttpOnly
< Set-Cookie: github_secret=bQpCEgEpmfTAKTjW; path=/; secure; HttpOnly
< X-Backend-Server: web2.bugs.scl3.mozilla.com
< Content-length: 0
< Content-Type: image/png; name="Fullscreen capture 01-01-2016 194503.png"; charset=
<
* Connection #0 to host bug1236152.bmoattachments.org left intact

Comment 1

[:glob ✱]

> select length(thedata) from attach_data where id=8703286
> 0

my guess is the conversion from bmp failed somehow, and it wasn't caught by BmpConvert extension.

Comment 2

[:glob ✱]

there are only two attachments impacted by this bug:
bug 1229904 attachment 8694893 [details]
bug 1236152 attachment 8703286 [details]

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1236161_2.patch

Added a lot more error checking to BmpConvert. In the event of errors, we bail out and don't change anything. Errors are logged with warn()

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 8707187 [details] [diff] [review]
1236161_2.patch

Review of attachment 8707187 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/BmpConvert/Extension.pm
@@ +54,5 @@
> +        # rewind so it can be read in again by other code
> +        seek($data, 0, SEEK_SET);
> +
> +        die "Error reading in BMP: $read_result"
> +          unless $read_result == 1;

According to the Image::Magick docs, $read_result will be undefined if successful. 
http://www.imagemagick.org/script/perl-magick.php#exceptions
Here you are checking for == 1 which causes all of my BMPs attached to be consider an error when writing. If I change the error code to:

my $read_error = $img->Read(file => \*$data);
seek($data, 0, SEEK_SET);
die "Error reading in BMP: $read_error" if $read_error;

and below:

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;

Then I can make the conversion happen without error. Did you see this same behavior?

@@ +82,4 @@
>  
>      ${$args->{data}} = $data;
>      $args->{attributes}->{mimetype} = 'image/png';
> +    $args->{attributes}->{filename} =~ s/\.bmp$/.png/i;

s/\.bmp$/\.png/i

Comment 5

[Dylan Hardison [:dylan] (he/him)]

From irc: 

11:09 <@dylan> https://gist.github.com/dylanwh/97e4497d87b5b9062be5
11:10 <@dylan> the result of Read() is a dualvar(), with an undefined value in the string part and a number in the number part
11:11 <@dylan> dualvars should never be used this way, but since they are...

Reference for dualvars: https://metacpan.org/pod/Scalar::Util#dualvar

Meanwhile, I found a bug with this (that also exists already): the size of converted attachments is always wrong.
How can we change the size of the attachment from inside the hook?

Comment 6

[David Lawrence [:dkl]]

(In reply to Dylan William Hardison [:dylan] from comment #5)
> From irc: 
> 
> 11:09 <@dylan> https://gist.github.com/dylanwh/97e4497d87b5b9062be5
> 11:10 <@dylan> the result of Read() is a dualvar(), with an undefined value
> in the string part and a number in the number part
> 11:11 <@dylan> dualvars should never be used this way, but since they are...
> 
> Reference for dualvars: https://metacpan.org/pod/Scalar::Util#dualvar

Ah. til;

> Meanwhile, I found a bug with this (that also exists already): the size of
> converted attachments is always wrong.
> How can we change the size of the attachment from inside the hook?

dkl> dylan: you *should* be able to just set the new size in $args->{attributes}->{attach_size}
dylan> cool! I will make a revision to the patch for that.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1236161_3.patch

I hope this code is a bit cleaner. It should work the same way. I had to lookup the boolean behavior of dualvars (https://github.com/chromatic/modern_perl_book/blob/master/sections/coercion.pod).

I also fixed the attachment size.

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8707973 [details] [diff] [review]
1236161_3.patch

Review of attachment 8707973 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/BmpConvert/Extension.pm
@@ +75,5 @@
> +          if $write_error;
> +
> +        $size = -s $tmp->filename;
> +        die "Error converting BMP to PNG results in empty file"
> +          if $size == 0;

For some reason I keep getting $size == 0 on my system. If I change UNLINK => 0, I can see the file in /tmp and it is not 0 bytes. 

attachment.cgi: Error converting BMP to PNG results in empty file at ./extensions/BmpConvert/Extension.pm line XX

The file looks fine as well:
/tmp/CkH5RPJZV3.png: PNG image data, 200 x 144, 8-bit colormap, non-interlaced

Dylan says it is working on his system so it may be environmental. Everything else looks fine. Any clues?

Comment 9

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1236161_4.patch

Good catch dkl! Using smaller files made me realize it was not flushing the file to disk. File::Temp has a ->flush method for this. Also there's a script to fix broken sizes in the db.

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8708636 [details] [diff] [review]
1236161_4.patch

Review of attachment 8708636 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

::: scripts/fix-attachment-sizes.pl
@@ +16,5 @@
> +
> +use Bugzilla;
> +use Bugzilla::Constants;
> +use Bugzilla::User;
> +use Bugzilla::User::APIKey;

Bugzilla::User and Bugzilla::User::APIKey are not used.

@@ +33,5 @@
> +          AND length(thedata) != attachments.attach_size });
> +say "Found ", scalar @$attachment_sizes, " attachments to fix";
> +
> +foreach my $attachment_size (@$attachment_sizes) {
> +    say "Fixing attachment $attachment_size->[1]";

say "Setting size for $attachment_size->[0] to $attachment_size->[1]";

Comment 11

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   050d08d..ace6728  master -> master