Last Comment Bug 1236161 - when converting a BMP attachment to PNG fails a zero byte attachment is created
: when converting a BMP attachment to PNG fails a zero byte attachment is created
Status: RESOLVED FIXED
:
Product: bugzilla.mozilla.org
Classification: Other
Component: General (show other bugs)
: Production
: Unspecified Unspecified
-- major (vote)
: ---
Assigned To: Dylan Hardison [:dylan]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-01 12:12 PST by YF (Yang)
Modified: 2016-01-19 19:59 PST (History)
3 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1236161_2.patch (2.28 KB, patch)
2016-01-12 15:29 PST, Dylan Hardison [:dylan]
dkl: review-
Details | Diff | Splinter Review
1236161_3.patch (2.78 KB, patch)
2016-01-14 10:07 PST, Dylan Hardison [:dylan]
dkl: review-
Details | Diff | Splinter Review
1236161_4.patch (4.48 KB, patch)
2016-01-15 19:04 PST, Dylan Hardison [:dylan]
dkl: review+
Details | Diff | Splinter Review

Description User image YF (Yang) 2016-01-01 12:12:07 PST
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 User image Byron Jones ‹:glob› 2016-01-04 21:29:54 PST
> 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 User image Byron Jones ‹:glob› 2016-01-11 20:10:17 PST
there are only two attachments impacted by this bug:
bug 1229904 attachment 8694893 [details]
bug 1236152 attachment 8703286 [details]
Comment 3 User image Dylan Hardison [:dylan] 2016-01-12 15:29:06 PST
Created attachment 8707187 [details] [diff] [review]
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 User image David Lawrence [:dkl] 2016-01-14 07:37:32 PST
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 User image Dylan Hardison [:dylan] 2016-01-14 08:42:01 PST
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 User image David Lawrence [:dkl] 2016-01-14 09:27:01 PST
(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 User image Dylan Hardison [:dylan] 2016-01-14 10:07:56 PST
Created attachment 8707973 [details] [diff] [review]
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 User image David Lawrence [:dkl] 2016-01-14 13:35:05 PST
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 User image Dylan Hardison [:dylan] 2016-01-15 19:04:48 PST
Created attachment 8708636 [details] [diff] [review]
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 User image David Lawrence [:dkl] 2016-01-19 12:28:49 PST
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 User image Dylan Hardison [:dylan] 2016-01-19 19:59:00 PST
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   050d08d..ace6728  master -> master

Note You need to log in before you can comment on or make changes to this bug.