Homebrew-core: Too conservative policy in ImageMagick 7.0.8-11_1

[adah1972]

• [X] are reporting a bug others will be able to reproduce and not asking a question. If you're not sure or want to ask a question do so on our Discourse: https://discourse.brew.sh
• [X] have a problem with brew install (or upgrade, reinstall) a single, official formula (not cask)? If it's a general brew problem please file this issue at Homebrew/brew: https://github.com/Homebrew/brew/issues/new/choose. If it's a brew cask problem please file this issue at https://github.com/Homebrew/homebrew-cask/issues/new/choose. If it's a tap (e.g. Homebrew/homebrew-php) problem please file this issue at the tap.
• [X] ran brew update and can still reproduce the problem?
• [X] ran brew doctor, fixed all issues and can still reproduce the problem?
• [X] ran brew gist-logs <formula> (where <formula> is the name of the formula that failed) and included the output link?

brew gist-logs imagemagick
https://gist.github.com/b3f53ef7f7fc52e267fb801b5c2568dc

• [ ] if brew gist-logs didn't work: ran brew config and brew doctor and included their output with your issue?

To help us debug your issue please explain:

• What you were trying to do (and why)

I am trying to shrink an image (relatively big at 4500x6000).

convert a.jpg -colorspace RGB -filter LanczosRadius -distort Resize '>3600x3600' -quality 92 -colorspace sRGB b.jpg

• What happened (include command output)

An error happened:

convert: cache resources exhausted `a.jpg' @ error/cache.c/OpenPixelCache/3873.

• What you expected to happen

Successful operation, of course.

It works in ImageMagick 7.0.8-8.

• Step-by-step reproduction instructions (by running brew install commands)

1. Find a big JPEG file (other kinds of file should do too); name it a.jpg.
2. convert a.jpg -colorspace RGB -filter LanczosRadius -distort Resize '>3600x3600' -quality 92 -colorspace sRGB b.jpg
3. An error message like mentioned above should show up.

• Suggestion for the fix

I see these line in the formula:

  # This isn't an upstream issue and this patch should not be removed.
  # Imagemagick delegate configuring secure defaults to users/packagers
  # and ship the most "open" (and thus potentially unsafe) configuration
  # possible out of the box. This policy is inspired by both Debian's and
  # the advice on Imagemagick's related page:
  # https://www.imagemagick.org/script/security-policy.php
  patch do
    url "https://raw.githubusercontent.com/Homebrew/formula-patches/a95f9dd/imagemagick/imagemagick_safer_defaults.diff"
    sha256 "3f22b13e206d2403b53692412b7b69d175530f5977350486b81da5027c548b44"
  end

The linked patch contains some extremely conservative policy:

-  <!-- <policy domain="resource" name="memory" value="2GiB"/> -->
-  <!-- <policy domain="resource" name="map" value="4GiB"/> -->
-  <!-- <policy domain="resource" name="width" value="10KP"/> -->
-  <!-- <policy domain="resource" name="height" value="10KP"/> -->
+  <policy domain="resource" name="memory" value="256MiB"/>
+  <policy domain="resource" name="map" value="512MiB"/>
+  <policy domain="resource" name="width" value="16KP"/>
+  <policy domain="resource" name="height" value="16KP"/>
…

It is exactly these policies that make my operation fail.

I feel very much surprised why anybody wants to limit the memory used by ImageMagick to only 256 MB. Currently the lowest configuration of Mac has 4 GB of RAM, I believe.

I think we should either get rid of the policy change, or use some more reasonable number in etc/ImageMagick-7/policy.xml, like:

  <policy domain="resource" name="memory" value="2GiB"/>
  <policy domain="resource" name="map" value="4GiB"/>
  <policy domain="resource" name="disk" value="8GiB"/>

(After these changes, convert works again.)

[DomT4]

I feel very much surprised why anybody wants to limit the memory used by ImageMagick to only 256 MB.

We're using the exact same numbers on this as Debian & the other Linux distros do, as well as what ImageMagick themselves recommend.

There's a security aspect to the limited memory usage, as running multiple convert commands on large jpg files without the cap is a fairly effective DoS attack, given doing so can result in freezes/hangs/crashes as the system tries to rejiggle its available memory to handle the load.

I think we should either get rid of the policy change, or use some more reasonable number in etc/ImageMagick-7/policy.xml

You can edit the numbers yourself locally. The policy is overwritten by Homebrew each version bump, but it's not overly arduous to replace the strings with sed, which if you set as a shell alias or function essentially then just becomes:

• brew upgrade
• "Oh there's a new imagemagick"
• imconfup/whatever you call the alias/function.

[fxcoudert]

The values above are, AFAICT, the Debian default policy. They are targeted toward a web server, apparently (because the comments discuss that many imagemagick processes will run in parallel). For a general purpose machine, it is indeed quite restrictive.

[fxcoudert]

Whatever values we enforce, we should probably find a way to make it so that if users modify their policy, an upgrade won't erase it. Especially as imagemagick releases a new version every other week.

[DomT4]

an upgrade won't erase it

It's been a deliberate decision on ImageMagick for it to be one of the few things Homebrew does retain config control over, FWIW. If people want to have that discussion again that's completely cool & justified, but it's something that isn't an accidental system.

[fxcoudert]

@DomT4 I remember the PR, and I wasn't against it. I didn't realise the values were so low, though. I have already hit this issue myself on my work machine this morning, for a file which is not outside of the ordinary. I think typical settings for a web server and for a single-user workstation maybe be pretty dissimilar.

If we keep these low values, I think we should switch to a mode of operation where they survive an upgrade, because a. it's always nicer for the user to have their wishes respected, b. imagemagick upgrade happen quite frequently.

[DomT4]

@fxcoudert I meant more historically rather than the recent PR, unless I'm misunderstanding the first line of your comment. We've patched the policy in the past, and I believe in general ImageMagick rely on the configuration directory being a "living" directory, which Homebrew isn't great at handling yet.

I'm open to compromise on the values, I think we can push them a bit, but I'm not keen on returning to the default settings. Somewhere in the middle between us & the default I could be okay with.

[adah1972]

I do not think DoS should be a major concern for a workstation, which is more likely the case for a Mac.

Anyway, I want the problem solved and not returning. And I am OK with just enlarging the numbers.

[DomT4]

With IPv6 it feels like we're entering an age where anything connected to the internet has to be afforded a pretty high level of security by default, barring people configuring firewalls sensibly _(which I'm not betting any significant sum of money on, heh)_.

Nonetheless, if you can fiddle with the numbers locally and come up with one that works & isn't as high as the default I'm open to it.

[adah1972]

How about the values I suggested in my original bug report? It works for me.

  <policy domain="resource" name="memory" value="2GiB"/>
  <policy domain="resource" name="map" value="4GiB"/>
  <policy domain="resource" name="disk" value="8GiB"/>

width and height might be loosened a bit too, as there is already another restriction on area. I guess 32KP might work better for people who play a lot with panoramas, but I do not have a strong opinion here.

[adah1972]

Do I need to take any further actions, like attaching the modified patch file here?

[fxcoudert]

If you could open a pull request, it would be very appreciated. Otherwise, one of the maintainers will find time to do it at some point…

[adah1972]

No harm to try modifying the patch, I think. :-)

imagemagick_safer_defaults.diff

[adah1972]

If you could open a pull request, it would be very appreciated. Otherwise, one of the maintainers will find time to do it at some point…

I do not have ideas how to modify/upload the formula-patches.

[fxcoudert]

The formula-patches part is done: https://github.com/Homebrew/formula-patches/commit/d9dc0a883945538b83c80dae6e106556a5609dc7

[MikeMcQuaid]

My suggestion here would be for this to be reverted and us to use the default upstream behaviour (reasons why here:
https://github.com/Homebrew/homebrew-core/pull/31558#issuecomment-420628039).

[fxcoudert]

@MikeMcQuaid I'm indifferent between 1. having a Homebrew policy, but not as strict as currently (given that it's upstream's documented recommendation), 2. shipping as is.

[MikeMcQuaid]

@Homebrew/maintainers anyone else have thoughts?

[lembacon]

I'm 👍 on reverting the patch.

[claui]

In favour of reverting the patch. It was @adah1972’s point on workstation vs. server use that ultimately convinced me:

I do not think DoS should be a major concern for a workstation, which is more likely the case for a Mac.

[commitay]

Opened https://github.com/Homebrew/homebrew-core/pull/32149 to revert.

[commitay]

Closed via https://github.com/Homebrew/homebrew-core/commit/16e1df6dc2657cae75996c15051139ed9a335ff6