[webkit-reviews] review granted: [Bug 31395] Bugzilla should show images in git patches : [Attachment 44312] Patch v1

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 31395: Bugzilla should show images in git patches
https://bugs.webkit.org/show_bug.cgi?id=31395

Attachment 44312: Patch v1
https://bugs.webkit.org/attachment.cgi?id=44312&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> +	   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.