Bugzilla should show images in git patches

• Component: WebKit Website
• Status: RESOLVED FIXED

[Shinichiro Hamaji]

It would be difficult to review expected images without this feature.

[Shinichiro Hamaji]

Created attachment 43038 [details]
Patch v0

[Shinichiro Hamaji]

Created attachment 43039 [details]
Example patch

[Shinichiro Hamaji]

Created attachment 43040 [details]
Example output

[Shinichiro Hamaji]

My patch is v0 because I don't think this will work as is. PrettyPatch.rb executes a perl script which is stored in WebKitTools/Scripts. I don't know how we can run scripts in WebKitTools from BugsSite. Could someone tell me how this can be done? I think we need to fix the following line in my patch:

GIT_DECODE_PATCH_PATH = "WebKitTools/Scripts/decode-git-binary-patch"

[Eric Seidel (no email)]

CCing folks who know something about bugs.webkit.org

[Adam Roben (:aroben)]

(In reply to comment #4)
> My patch is v0 because I don't think this will work as is. PrettyPatch.rb
> executes a perl script which is stored in WebKitTools/Scripts. I don't know how
> we can run scripts in WebKitTools from BugsSite. Could someone tell me how this
> can be done? I think we need to fix the following line in my patch:
> 
> GIT_DECODE_PATCH_PATH = "WebKitTools/Scripts/decode-git-binary-patch"

You're right that this won't work on bugs.webkit.org. On the server, we only have access to the BugsSite directory.

Maybe we could change the server to also have a checkout of WebKitTools/Scripts in a known location. I'll defer to Mark and Bill on that one.

[Shinichiro Hamaji]

Thanks Eric for adding CC and thanks Adam for your comment.

> You're right that this won't work on bugs.webkit.org. On the server, we only
> have access to the BugsSite directory.
> 
> Maybe we could change the server to also have a checkout of WebKitTools/Scripts
> in a known location. I'll defer to Mark and Bill on that one.

I see. Checking out WebKitTools/Scripts would be nice. With this change, we may be able to run check-webkit-style in bugzilla, too. It will help people to notice trivial style issues without waiting reviewers' comments.

[Shinichiro Hamaji]

Ping? I (and maybe most of git users) really want to use git even for patches with images. Maybe I should talk in IRC, but it's not easy for me to be there due to time difference.

[Eric Seidel (no email)]

You can try emailing Mark or Bill, but those are the decision makers here if you require server-side changes.

[Adam Barth]

Attachment 43038 [details] passed the style-queue

[David Kilzer (:ddkilzer)]

Comment on attachment 43038 [details]
Patch v0

Rather than writing a new decode-git-binary-patch script which uses VCSUtils::decodeGitBinaryPatch(), I think the PrettyPatch.rb script should instead use a git command to do its decoding work directly.  We can guarantee that git will be installed on bugs.webkit.org, so we don't have to rely on a Perl implementation of the git decoding algorithm to get the image data.

> diff --git a/BugsSite/PrettyPatch/PrettyPatch.rb b/BugsSite/PrettyPatch/PrettyPatch.rb
>              lines_with_contents = lines[startOfSections...lines.length]
>              @sections = DiffSection.parse(lines_with_contents) unless @binary
> -            @image_url = "data:image/png;base64," + lines_with_contents.join if @image
> +            if @image
> +                @image_url = "data:image/png;base64," + lines_with_contents.join
> +            elsif @git_image
> +                begin
> +                    stdin, stdout, stderr = *Open3.popen3(GIT_DECODE_PATCH_PATH)
> +                    stdin.puts(lines)
> +                    stdin.close
> +                    @image_urls = stdout.readlines
> +                    stdout.close
> +                    error = stderr.read
> +                    stderr.close
> +                    if error != ''
> +                        @image_error = "Failed to decode git binary patch<pre>#{CGI.escapeHTML(error)}</pre>"
> +                    end
> +                rescue
> +                    @image_error = "Exception raised during decoding git binary patch: #{$!}"

The error message, $!, needs to be HTML-escaped as well.

r- to fix the HTML-escaping issue noted and to use a git command directly to decode the binary patch.

[Shinichiro Hamaji]

Created attachment 44312 [details]
Patch v1

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44312 [details] without any errors.

[Shinichiro Hamaji]

I updated my patch. The new patch uses git as suggested by David (thanks!). I
couldn't find a git command to extract a file from git binary patch, so my
patch does:

1. Use Tempfile to create a unique name.
2. Append ".bin" to the filename so that the file doesn't exist.
3. Create a git patch which creates a new file using the filename we've got.
4. Use git-apply to extract the image file.
5. Read the contents of the image created.
6. Remove the file we've created.

So, this patch depends /tmp is writable. Can PrettyPrint.rb create a file in
/tmp? If not, I will need to figure out another way...

[William Siegrist]

Yes, /tmp is writable.

[Shinichiro Hamaji]

(In reply to comment #15)
> Yes, /tmp is writable.

Nice. Thanks for your reply. Another concern is the version of Ruby. I checked my script is working with ruby-1.8.6 on Leopard and ruby-1.8.7 on Linux.

[William Siegrist]

PrettyPatch uses the stock ruby (Leopard, 1.8.6).

[David Kilzer (:ddkilzer)]

Comment on attachment 44312 [details]
Patch v1

> +        def self.extract_contents_from_git_binary_chunk(encoded_chunk, git_index)
> +            # We use Tempfile we need a unique file among processes.
> +            tempfile = Tempfile.new("PrettyPatch")
> +            # We need a filename which doesn't exist to apply a patch
> +            # which creates a new file. Append a suffix so filename
> +            # doesn't exist.
> +            filename = File.basename(tempfile.path) + '.bin'

Prepending ".bin" to the unique filename may (potentially) lead to a symlink attack because there is a short time between the patch file being written and the patch being applied.  It would probably be best to generate the "bin" filename as well.

> +            patch = FileDiff.git_new_file_binary_patch(filename, encoded_chunk, git_index)
> +
> +            # Apply the git binary patch using git-apply.
> +            Dir.chdir(File.dirname(tempfile.path)) do
> +                stdin, stdout, stderr = *Open3.popen3(GIT_PATH + " apply")
> +                begin
> +                    stdin.puts(patch)
> +                    stdin.close
> +
> +                    error = stderr.read
> +                    raise error if error != ""
> +
> +                    contents = File.read(filename)
> +                ensure
> +                    stdin.close unless stdin.closed?
> +                    stdout.close
> +                    stderr.close
> +                    File.unlink(filename) if File.exists?(filename)
> +                end
> +
> +                return nil if contents.empty?
> +                return "data:image/png;base64," + [contents].pack("m")
> +            end
> +        end
>      end

I don't see tempfile being unlinked anywhere, or is that implicit when its object is deleted?  If not, it needs to be removed as well.

r=me if you fix the above two issues.

[Shinichiro Hamaji]

Thanks for your review. I understand your concern, but I think they are not real issues. I'd like to go as is if there are no other concerns. David, can you check my comments below? If there are something unclear, please ask again.

> Prepending ".bin" to the unique filename may (potentially) lead to a symlink
> attack because there is a short time between the patch file being written and
> the patch being applied.  It would probably be best to generate the "bin"
> filename as well.

If we create "bin" filename, we need to calculate git index, which is probably sha1sum and depends on the filename, to create the patch. We don't need to calculate the git index if the original file is not existed. We can just use 0000000000000000000000000000000000000000 as the git index of the original file. That's why my patch doesn't just use tempfile but append ".bin" to the filename.

Anyway, it seems git-apply doesn't modify the file if there is already a file or a symlink and the attempt just fails. So, I think there are no security risk.

> I don't see tempfile being unlinked anywhere, or is that implicit when its
> object is deleted?  If not, it needs to be removed as well.

Yeah, the tempfile will be removed when the object is deleted.

Thanks!

[David Kilzer (:ddkilzer)]

(In reply to comment #19)
> Thanks for your review. I understand your concern, but I think they are not
> real issues. I'd like to go as is if there are no other concerns. David, can
> you check my comments below? If there are something unclear, please ask again.

Okay, sounds good.  r=me

[Shinichiro Hamaji]

Comment on attachment 44312 [details]
Patch v1

Clearing flags on attachment: 44312

Committed r51748: <http://trac.webkit.org/changeset/51748>

[Shinichiro Hamaji]

All reviewed patches have been landed.  Closing bug.

[Shinichiro Hamaji]

Hmm... my patch raises an exception. As I have no fix for now, I'll revert my change. The exception was:

Exception raised during decoding git binary patch:

/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/open3.rb:67: warning: Insecure world writable dir /private/tmp/ in PATH, mode 041777

[Shinichiro Hamaji]

Created attachment 44390 [details]
Patch v2

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44390 [details] without any errors.

[Shinichiro Hamaji]

I think This issue happened because Bugzilla.pm sets PATH="". When PATH environment value is empty, the current directory is considered in PATH. As the Patch v1 changes directory to /tmp, Ruby produced the warning when it runs an external command. As there is a command line flag --directory in git-apply, using this flag would be the best solution.

[David Kilzer (:ddkilzer)]

Comment on attachment 44390 [details]
Patch v2

r=me

[Shinichiro Hamaji]

Comment on attachment 44390 [details]
Patch v2

Clearing flags on attachment: 44390

Committed r51826: <http://trac.webkit.org/changeset/51826>

[Shinichiro Hamaji]

All reviewed patches have been landed.  Closing bug.

[Shinichiro Hamaji]

Thanks for you review. OK, now it seems working!

https://bugs.webkit.org/attachment.cgi?id=43039&action=prettypatch

Thanks again folks for several suggestions.

[Eric Seidel (no email)]

The pretty-patch link you reference has this in it:

LayoutTests/platform/mac/fast/css/corrupted.png

Exception raised during decoding git binary patch:
error: corrupt binary patch at line 15: junk_line

./PrettyPatch/PrettyPatch.rb:343:in `extract_contents_from_git_binary_chunk'
./PrettyPatch/PrettyPatch.rb:258:in `initialize'
./PrettyPatch/PrettyPatch.rb:257:in `collect'
./PrettyPatch/PrettyPatch.rb:257:in `initialize'
./PrettyPatch/PrettyPatch.rb:309:in `new'
./PrettyPatch/PrettyPatch.rb:309:in `parse'
./PrettyPatch/PrettyPatch.rb:309:in `collect'
./PrettyPatch/PrettyPatch.rb:309:in `parse'
./PrettyPatch/PrettyPatch.rb:15:in `prettify'
PrettyPatch/prettify.rb:26

[David Kilzer (:ddkilzer)]

(In reply to comment #31)
> The pretty-patch link you reference has this in it:
> 
> LayoutTests/platform/mac/fast/css/corrupted.png
> 
> Exception raised during decoding git binary patch:
> error: corrupt binary patch at line 15: junk_line

I'm pretty sure that was done on purpose for error handling.

[Shinichiro Hamaji]

> I'm pretty sure that was done on purpose for error handling.

Yeah, I intentionally inserted this junk_line to see how corrupt patch is handled.