toy / image_optim

Optimize images using multiple utilities
https://github.com/toy/image_optim
MIT License
1.52k stars 109 forks source link

Invalid cross-device link #180

Closed muk-ai closed 4 years ago

muk-ai commented 4 years ago

I'll leave a report for anyone who encounters the same error.

error message

Invalid cross-device link @ rb_file_s_rename - (/tmp/fat20200909-9154-12oecct.png, /var/www/my-app-name/releases/20200909065727/tmp/1599636588-64606488395367-0002-2349/fat.png)

related gems and configuration

systemd is configured to give nginx a private /tmp.

PrivateTmp=true

why the error occurred

  1. carrierwave saves the uploaded file to /var/www/my-app-name/releases/20200909065727/tmp. (it is Rails.root.join('tmp'))
  2. image_optim tries to create a hard link in /tmp when processing the file.
  3. Since /var/www/my-app-name/releases/2020090909065727/tmp and /tmp have different filesystem namespaces, "Invalid cross-device link" error occurs.

our solution

Changed tmp directory path by specifying ENV['TMPDIR'] in order to use the same tmp directory. Or turn off the PrivateTmp feature.

toy commented 4 years ago

Thank you for reporting! Can you please confirm that the bug was introduced in 0.27.0? Does it work without specifying TMPDIR using version 0.26.5? Also is /var/www/my-app-name/releases/20200909065727/tmp or /tmp a symlink? I'm trying to understand if same device check is wrong. Is it also possible to share a backtrace of the exception?

muk-ai commented 4 years ago

First of all, thank you for maintaining this gem.

Can you please confirm that the bug was introduced in 0.27.0?

An error occurred on 0.27.0. We had no problems with 0.26.5.

Also is /var/www/my-app-name/releases/20200909065727/tmp or /tmp a symlink?

They are not symlinks. However, we are using the following symlink to release app. /var/www/my-app-name/current -> /var/www/my-app-name/releases/20200909065727

Is it also possible to share a backtrace of the exception?

I hope this helps.

/GEM_ROOT/gems/image_optim-0.27.0/lib/image_optim/path.rb:56→ rename
/GEM_ROOT/gems/image_optim-0.27.0/lib/image_optim/path.rb:56→ rename
/GEM_ROOT/gems/image_optim-0.27.0/lib/image_optim/path.rb:56→ replace
/home/ec2-user/.rbenv/versions/2.7.1/lib/ruby/2.7.0/delegate.rb:343→ block in delegating_block
/GEM_ROOT/gems/image_optim-0.27.0/lib/image_optim.rb:133→ optimize_image!
/PROJECT_ROOT/app/uploaders/base_uploader.rb:24→ optimize
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/uploader/processing.rb:83→ block (2 levels) in process!
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/uploader/processing.rb:75→ each
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/uploader/processing.rb:75→ block in process!
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/uploader/callbacks.rb:15→ with_callbacks
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/uploader/processing.rb:74→ process!
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/uploader/callbacks.rb:14→ block in with_callbacks
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/uploader/callbacks.rb:14→ each
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/uploader/callbacks.rb:14→ with_callbacks
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/uploader/cache.rb:144→ cache!
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/mounter.rb:63→ block (2 levels) in cache
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/mounter.rb:176→ handle_error
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/mounter.rb:47→ block in cache
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/mounter.rb:46→ map
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/mounter.rb:46→ cache
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/mount.rb:146→ opus_image=
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/mount.rb:373→ opus_image=
/GEM_ROOT/gems/carrierwave-2.1.0/lib/carrierwave/orm/activerecord.rb:75→ opus_image=
 (*snipped*)
toy commented 4 years ago

After few attempts to reproduce it using docker and vagrant I still can't find how "same device" check would be true for temp file path and destination directory. For your original example following should be true File.stat('/tmp/fat20200909-9154-12oecct.png').dev == File.stat('/var/www/my-app-name/releases/20200909065727/tmp/1599636588-64606488395367-0002-2349').dev. But /tmp should already be on tempfs and PrivateTmp=true if I understood correctly mounts subfolder from that temporary file system as /tmp for the process, so devices should have different identifier.

Can you come up with reproducible example/environment?

muk-ai commented 4 years ago

We didn't use /tmp as tmpfs. (Amazon Linux 2 image)

$ LANG=C df -hT
Filesystem     Type      Size  Used Avail Use% Mounted on
devtmpfs       devtmpfs  1.9G     0  1.9G   0% /dev
tmpfs          tmpfs     1.9G     0  1.9G   0% /dev/shm
tmpfs          tmpfs     1.9G  408K  1.9G   1% /run
tmpfs          tmpfs     1.9G     0  1.9G   0% /sys/fs/cgroup
/dev/nvme0n1p1 xfs        40G   33G  7.6G  82% /
tmpfs          tmpfs     389M     0  389M   0% /run/user/1000
$

Can you come up with reproducible example/environment?

It may take some time, but I try to prepare a reproduction environment.

muk-ai commented 4 years ago

I created a minimal reproducible example. https://github.com/muk-ai/image_optim_namespace_repro To be safe, please run it in a Linux virtual machine.

step 1. start the bash process in an isolated namespace.

$ sudo unshare --mount /bin/bash

step 2. create a temporary directory and mount it as /tmp.

# PRIVATE_TMPDIR=$(mktemp -d)
# mount --bind $PRIVATE_TMPDIR /tmp

step 3. create a hard link.

# LANG=C ln image.png /tmp/hardlink
ln: failed to create hard link '/tmp/hardlink' => 'image.png': Invalid cross-device link

step 4. make sure that the same error occurs with optimize_image.

# bundle exec ruby optimize.rb
WARN: advpng 1.15 at /bin/advpng (< 1.17) does not use zopfli
Traceback (most recent call last):
    5: from optimize.rb:12:in `<main>'
    4: from /usr/local/rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/image_optim-0.27.0/lib/image_optim.rb:133:in `optimize_image!'
    3: from /usr/local/rbenv/versions/2.7.1/lib/ruby/2.7.0/delegate.rb:343:in `block in delegating_block'
    2: from /usr/local/rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/image_optim-0.27.0/lib/image_optim/path.rb:56:in `replace'
    1: from /usr/local/rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/image_optim-0.27.0/lib/image_optim/path.rb:56:in `rename'
/usr/local/rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/image_optim-0.27.0/lib/image_optim/path.rb:56:in `rename': Invalid cross-device link @ rb_file_s_rename - (/tmp/image20200918-15809-1nfzntw.png, ./image.png) (Errno::EXDEV)
toy commented 4 years ago

Thanks for creating the example!

I was able to test that mounting directory from same file system will preserve device id but will create a boundary for creating links. This means that checking File::Stat#dev in ruby code will not be enough. Additionally looking at source of mv (freebsd, gnu mv -> gnu copy) it looks like rescuing Errno::EXDEV and copying file is the right thing to do. I'm wondering if it is better also to show a warning or consider it a normal behaviour.

muk-ai commented 4 years ago

Thanks for also checking out the mv behavior. I don't think a warning is necessary. Because mv does not show any warning.

toy commented 4 years ago

Can you please check if changes in branch fix-180 fix the problem?

muk-ai commented 4 years ago

It works perfectly in PrivateTmp enabled environment :tada: On behalf of our team, I would like to thank you for support of this edge case.

toy commented 4 years ago

Released version 0.27.1