Differential D7969

Prevents crash
ClosedPublic
Actions

Authored by poke1024 on Sep 24 2017, 1:04 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Commits
R37:6cacb1096f1a: Prevent a crash when swap file is not created on OSX
R37:4446c5120fb8: Prevent a crash when swap file is not created on OSX
Summary

Prevents a random crash with Krita 3.2.1 on Mac OS X 10.12.6 (not reproducible).

The crash happened in memcpy in KisSwappedDataStore::swapOutTileData; see attach crash log:

krita-crash.txt90 KBDownload

Since the crash is a clean null pointer memory fault, and since m_buffer.data() is highly unlikely to be null and it's allocated a few lines before using m_buffer.resize(), I highly suspect ptr, i.e. m_swapSpace->getWriteChunkPtr(chunk) to be the culprit here.

The only case I can imagine this goes wrong, if m_file.map in KisMemoryWindow::adjustWindow yields null; indeed the QT docs say that QTemporaryFile::map may return 0 here (http://doc.qt.io/qt-5/qfiledevice.html#map).

A better fix than this PR would actually deal properly with the situation of not being able to swap out or swap in tiles (without having to abort the program). Two ideas for this:

(1) half the chunk size in KisMemoryWindow::adjustWindow until map succeeds?
(2) make KisTileDataStore::trySwapTileData just fail silently? (But what to do with swap ins?)

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
poke1024 created this revision.Sep 24 2017, 1:04 PM
alvinhochun added a subscriber: alvinhochun.Sep 24 2017, 2:47 PM

Heh, so not only does OS X impose a tight shared memory size, but also memory-mapped files?

Is QFileDevice::map used anywhere else in the code?

poke1024 added a comment.Sep 24 2017, 4:05 PM

OK, so it turns out my "fix" just produces another crash; the fix will be then to really fix it somehow:

krita-crash-2.txt103 KBDownload

@alvinhochun I was aware that iOS imposes limits (see https://stackoverflow.com/questions/13425558/why-does-mmap-fail-on-ios), but I wasn't aware that OS X does. But it seems exactly so.

Concerning other calls to QFileDevice::map: hard to tell, as there are so many map() calls in Krita. Might compile against a patched QT to find out.

dkazakov added a subscriber: dkazakov.Sep 25 2017, 1:14 PM

Hi, @poke1024!

  1. The default size of the mapping window is 16MiB, so the idea about memory fragmentation sounds really strange. The swapping store uses a standard "clock" algorithm for the swap space allocation, iterating over 64-MiB slabs with two 16-MiB windows: one for reading and another one for writing.
  1. You can also try to check if(requestedChunk.size() > windowSize) line for execution, that is the only (bun almost impossible) way how Krita can request more memory in one window.
  1. Btw, you can also try to activate line m_file.exists() and check if it helps a little bit. Qt does quite weird things on different architectures.
  1. About fallback strategies, there are two strategies:
    1. Decrease the window size until the mapping succeeds
    2. Return the failure to upper levels.
      • for swap-out: just cancel the swap-out process (with issuing a warning)
      • for swap-in: fallback to normal file read/write (reaally a lot of work on refactoring)
dkazakov requested changes to this revision.Sep 25 2017, 2:29 PM

I'll mark the patch as needs changes...

This revision now requires changes to proceed.Sep 25 2017, 2:29 PM
poke1024 updated this revision to Diff 19898.EditedSep 25 2017, 3:40 PM

OK, so the problem on OS X turns out NOT to be related to mmap size limits, but simply a problem in creating and opening the swapfile. The swapfile does not get created properly on OS X and thus mmap fails later on, as there's no file to mmap to.

The reason the swapfile does not get created is that OS X stores each temp file in its own garbled path (e.g. /var/folders/6k/9953vs0513ldszxq8gqjz4mh0000gq/T/KRITA_SWAP_FILE_XXXXXX). Thus the usual logic implemented in Krita for getting "one" permanent (i.e. saved in config file) correct tmp path and then creating a swap file there, is not viable on OS X with OS X's tmp file folder management.

On OS X, you can get a temp path for a temp file, but when you create the swap file there later on, the path might already be invalid (this is also sort of a QT QTemporaryFile problem IMHO, but anyway).

This new diff fixes swapfiles on OS X through these changes:

  • Clear all existing configs on OS X that point swapfiles to /var/folders (the internal OS X tmp file archive that must not be stored in a config file, as it is definitely not usually writable or readable across app starts) and make Krita revert to the "default" on such installations
  • Use the home directory on OS X as default swapfile dir, as there is no dedicated tmp file location that allows for setting the swap file name (users can always specify a better swapfile location, but the home dir at least makes it clear where to delete the thing should Krita crash, as opposed to some unnamed large file deep down somewhere in /var/folders/.../.../...).

Additionally, this latest diff adds proper error checking on swapfile resizes and swapfile mmap, which should be beneficial for all platforms. Currently if any of these fail, Krita will crash. With these changes, swapping out will just noop gracefully (and print out a warning).

Swapping in still might crash, but this should be an extremely rare case, as it means that a swap out did happen before successfully and now the swap in does not succeed (which would mean: the mmap for the write worked, but now the mmap for the read doesn't, which really is improbable IMHO). It adds an assert that should identify these crashes clearly should they ever happen in the real world.

dkazakov added a comment.Sep 26 2017, 6:30 AM

Hi, @poke1024!

Could you please check if OSX automatically deletes the temporary folder if Krita crashes (or killed with Ctrl+C)?

If the temporary folder is automatically deleted, then we could still put the swap file into the temp folder... Because the only reason for the swap file to have a meaningful name is to let the user/developer to clean up his disk space after Krita crashes.

poke1024 added a comment.Sep 26 2017, 7:45 AM

@dkazakov, the temp folders should eventually get deleted, but it might take days, months or years, and it's not transparent when OS X does this (and not documented). For example, here's my temp folder where krita would store temp files if one disabled the swap file template file name after force quitting Krita two times:

bernhard$ ls -altr /var/folders/pp/n5gfd7pd5r924mt65zdnm7b00000gp/T
total 5216
drwxr-xr-x@ 5 bernhard staff 170 22 Aug 06:17 ..
drwx------ 2 bernhard staff 68 24 Sep 21:39 com.apple.bird
drwx------ 2 bernhard staff 68 24 Sep 21:39 com.apple.tccd
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.soagent
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.iTunesCacheExtension
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.iBooksX.CacheDelete
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.cloudphotosd
drwx------@ 3 bernhard staff 102 24 Sep 21:39 com.apple.Safari.CacheDeleteExtension
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.MailCacheDelete
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.AddressBook.ContactsAccountsService
drwx------ 2 bernhard staff 68 24 Sep 21:39 .CalendarLocks
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.ctkahp
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.CloudPhotosConfiguration
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.CalendarNotification.CalNCService
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.getdropbox.dropbox.garcon
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.inputmethod.EmojiFunctionRowItem
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.dt.IDECacheDeleteAppExtension
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.AirPlayUIAgent
drwx------@ 2 bernhard staff 68 24 Sep 21:39 de.pointworks.CustomMenu-Helper
drwx------@ 2 bernhard staff 68 24 Sep 21:39 at.obdev.MicroSnitchOpenAtLoginHelper
drwx------@ 3 bernhard staff 102 24 Sep 21:39 de.pointworks.CustomMenu-3
-rw-r--r-- 1 bernhard staff 0 24 Sep 21:39 .keystoneAgentLock
drwx------ 4 bernhard staff 136 24 Sep 21:39 .AddressBookLocks
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.photolibraryd
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.SocialPushAgent
drwx------@ 3 bernhard staff 102 24 Sep 21:39 com.apple.CalendarAgent
drwx------ 2 bernhard staff 68 24 Sep 21:39 assistantd
drwx------ 2 bernhard staff 68 24 Sep 21:39 com.apple.Safari.History
drwx------@ 2 bernhard staff 68 24 Sep 21:39 com.apple.lateragent
drwx------@ 3 bernhard staff 102 24 Sep 21:40 com.apple.geod
drwx------@ 3 bernhard staff 102 24 Sep 21:40 chrome-pfJPl7
drwx------@ 4 bernhard staff 136 24 Sep 21:40 .com.google.Chrome.0hkMBs
-rw------- 1 bernhard staff 846 24 Sep 21:54 xcrun_db
-rw-r--r-- 1 bernhard staff 0 24 Sep 22:14 qtsingleapplication-2a7f-1f6-lockfile
drwx------@ 2 bernhard staff 68 24 Sep 22:14 com.apple.quicklook.ui.helper
srwxr-xr-x 1 bernhard staff 0 24 Sep 22:14 qtsingleapplication-2a7f-1f6
drwxr-xr-x 5 bernhard staff 170 24 Sep 22:18 qt_menu.nib
drwx------@ 2 bernhard staff 68 25 Sep 09:48 com.apple.ncplugin.stocks
drwx------@ 2 bernhard staff 68 25 Sep 09:48 com.apple.ncplugin.weather
drwx------@ 2 bernhard staff 68 25 Sep 09:48 com.apple.iCal.CalendarNC
-rw------- 1 bernhard staff 1024 25 Sep 09:51 krita.d28568
-rw------- 1 bernhard staff 0 25 Sep 09:52 krita.X33285
-rw------- 1 bernhard staff 1024 25 Sep 09:52 krita.Q33285
drwx------@ 2 bernhard staff 68 25 Sep 10:11 com.apple.TextEdit
-rw-r--r-- 1 bernhard staff 0 25 Sep 17:02 qtsingleapplication-a6e3-1f6-lockfile
srwxr-xr-x 1 bernhard staff 0 25 Sep 17:02 qtsingleapplication-a6e3-1f6
drwx------@ 2 bernhard staff 68 25 Sep 18:19 com.apple.OSDUIHelper
drwxr-xr-x@ 3 bernhard staff 102 25 Sep 18:31 com.apple.Safari
drwx------ 2 bernhard staff 68 25 Sep 18:31 com.apple.Safari.SearchHelper
drwx------@ 2 bernhard staff 68 25 Sep 18:34 com.apple.siri.media-indexer
drwx------@ 2 bernhard staff 68 25 Sep 18:34 com.apple.photoanalysisd
drwx------@ 2 bernhard staff 68 25 Sep 19:50 com.apple.mediaanalysisd
-rw------- 1 bernhard staff 22416 25 Sep 22:19 +~JF1184254371099382522.tmp
-rw------- 1 bernhard staff 221184 25 Sep 22:20 +~JF8381538922050533600.tmp
-rw------- 1 bernhard staff 162976 25 Sep 22:20 +~JF7870759322329908070.tmp
-rw------- 1 bernhard staff 218476 25 Sep 22:20 +~JF6892111847622721200.tmp
-rw------- 1 bernhard staff 197644 25 Sep 22:20 +~JF5815926952455793658.tmp
-rw------- 1 bernhard staff 252032 25 Sep 22:20 +~JF5005119267312603151.tmp
-rw------- 1 bernhard staff 197004 25 Sep 22:20 +~JF4652139972710234715.tmp
-rw------- 1 bernhard staff 220772 25 Sep 22:20 +~JF4058713504330616569.tmp
-rw------- 1 bernhard staff 163624 25 Sep 22:20 +~JF2603844122783701120.tmp
-rw------- 1 bernhard staff 220044 25 Sep 22:20 +~JF167718720007578565.tmp
drwxr-xr-x 3 bernhard staff 102 25 Sep 22:20 hsperfdata_bernhard
-rw------- 1 bernhard staff 0 25 Sep 23:21 KRITA_SWAP_FILE_k47028
-rwxr-xr-x 1 bernhard staff 190576 26 Sep 08:27 .com.electron.brave.D80wao
-rwxr-xr-x 1 bernhard staff 42420 26 Sep 08:27 .com.electron.brave.akAv1T
-rwxr-xr-x 1 bernhard staff 499972 26 Sep 08:27 .com.electron.brave.JUyZc6
-rwxr-xr-x 1 bernhard staff 21184 26 Sep 08:27 .com.electron.brave.FgoSkl
drwx------ 4 bernhard staff 136 26 Sep 08:27 .com.electron.brave.8I2ZlV
drwx------ 2 bernhard staff 68 26 Sep 08:39 TemporaryItems
drwx------@ 2 bernhard staff 68 26 Sep 08:42 com.apple.DataDetectorsLocalSources
-rw------- 1 bernhard staff 0 26 Sep 09:35 krita.w92547
drwx------@ 73 bernhard staff 2482 26 Sep 09:37 .

The files starting with " krita." were produced starting Krita and then terminating with ctrl-c as you suggested. No cleanup here. And lots of other garbage :-) For a non-developer user this is a nightmare.

As far as I'm aware there are no docs from Apple on how and when these folders will eventually get deleted. The web is full of reports of people having huge temp folders growing and not knowing what to do (see e.g. https://apple.stackexchange.com/questions/209266/el-capitan-private-var-folders-cache-files-consuming-30-40-gb).

dkazakov accepted this revision.Oct 2 2017, 7:24 PM

Hi, @poke1024!

The patch looks fine now. I will push it under your name.

Just two small comments:

  1. Next time you build Krita, please make sure you add -DBUILD_TESTING=on to the cmake command line. The patch fails to build with the unittests enabled (I'll fix it before pushing).
  2. Make sure your editor is configured to use '4 spaces' for indentation instead of tabs :)
This revision is now accepted and ready to land.Oct 2 2017, 7:24 PM

Revision Contents
Changeset List

Diff 19898

View Options

libs/image/kis_image_config.cpp

Loading...
View Options

libs/image/tiles3/kis_tile_data_store.cc

Loading...
View Options

libs/image/tiles3/swap/kis_memory_window.h

Loading...
View Options

libs/image/tiles3/swap/kis_memory_window.cpp

Loading...
View Options

libs/image/tiles3/swap/kis_swapped_data_store.h

Loading...
View Options

libs/image/tiles3/swap/kis_swapped_data_store.cpp

Loading...