Calibre consumes all memory then crashes when connecting an MTP device

[Karmix]

There is a problem where connecting a new device via MTP causes Calibre to consume all available memory until the system crashes or an OOM process kills Calibre. This is because Calibe's MTP driver tries to use Storage IDs and Object Handles interchangeably. While both identifiers are defined as UINT32s, they represent different things.

Presently, the constructor for the FilesystemCache class enumerates all of the Storage Locations on a device and creates FileOrFolder entries for them in the cache. (https://github.com/kovidgoyal/calibre/blob/2bb2573ff8a625f5428ba3f0c7a40a9c8b7899ed/src/calibre/devices/mtp/filesystem_cache.py#L219-L226)

The constructor for the FileOrFolder class then misinterprets the `id` field on the Storage Location as an Object Handle
(https://github.com/kovidgoyal/calibre/blob/2bb2573ff8a625f5428ba3f0c7a40a9c8b7899ed/src/calibre/devices/mtp/filesystem_cache.py#L42)
instead of a Storage ID and adds it to the the cache's object index in `id_map`. (https://github.com/kovidgoyal/calibre/blob/2bb2573ff8a625f5428ba3f0c7a40a9c8b7899ed/src/calibre/devices/mtp/filesystem_cache.py#L77)

Later, when FileOrFolder instances are created for filesystem objects, the FileOrFolder constructor relocates objects at the root of the file hierarchy to reside under the Storage Location object pre-populated in the filesystem cache. (https://github.com/kovidgoyal/calibre/blob/2bb2573ff8a625f5428ba3f0c7a40a9c8b7899ed/src/calibre/devices/mtp/filesystem_cache.py#L68-L69)

I suspect this done in an attempt to prefix the path for objects in the filesystem with the name of the storage location under which they reside (e.g. MainStorage/absolute/path/to/a/file). The problem with this approach is that Storage Locations are not assigned Object Handles because they are not objects in the file hierarchy, and using the Storage ID as an Object Handle only works until an actual filesystem object is assigned that Object Handle.

When an object is added to the cache with an Object Handle that is numerically equivalent to one of the Storage IDs, the FileOrFolder constructor will replace the pre-populated Storage Location in the cache's object index (`id_map`) with the new object. Because the objects in the root of the filesystem were relocated to reside under the Storage Location, replacing it makes the new object an ancestor of all objects in the file system, including itself, creating a cyclical reference.

Afterward, calls to `full_path` on FileOrFolder instances will attempt to traverse the cyclical reference and construct what would be an infinitely long string until it consumes all of the system's free memory and Calibre crashes.

Most MTP implementations that I have seen sequentially assign Object Handles starting with 1 to objects as they are encountered in an MTP session. Once 65536 objects have been assigned handles, the implementations starts assigning values that may be numerically identical to Storage IDs (i.e. the first logical partition of the first writable storage location would be assigned the Storage ID 0x00010001 = 65537). Reproducing this issue will be specific to implementation details and Storage IDs for each MTP device, but in most cases, populating the filesystem on the primary storage location with at least 65537 files and folders and removing Calibre's driveinfo and metadata files should be sufficient.

I'll attach a patch that I have used to address the issue when working with my device.

[Kovid Goyal (kovid)]

Hmm, well it's on my TODO list to rewrite the FilesystemCache to better
support multiple storages (currently IIRC it uses a single id_map for
all storages, which depending on MTP implementation might cause object id
clashes between storages). When I do that, I will fix object_id clashes
between storage ids and filesystem objects as well.

[Karmix (doug-knight)]

Object Handles are required to be unique across all Storage Locations on the device for each session, so storing them all in the same map shouldn't create clashes (unless the device broke with the standard, which happens, are you seeing devices with this problem?). The issue the patch addressed was that Storage Locations were getting mixed in there, too. The Storage Locations were already stored separately in the FilesystemCache's `entries` member variable, so removing them from `id_map` and updating the logic for parent objects resolved the issue.

I did see that the current implementation could use a lot of refactoring, so I tried to keep the patch minimal and easy for you to review. When investigating, I saw conversations you were having with several other people about the same issue. If you don't mind merging this in the mean-time, I suspect it will help a few people.

[Kovid Goyal (kovid)]

Fixed in branch master. The fix will be in the next release. calibre is usually released every alternate Friday.

[Kovid Goyal (kovid)]

It's already fixed in master. Feel free to test.

[Karmix (doug-knight)]

I have tested, and it appears to be working. Thank you.