Handling idevicerestore program in multi-threaded applications

[Leon-cam]

Description

I'm encountering an issue when using idevicerestore in a multi-threaded application. The problem arises after the idevicerestore_start function finishes restoring an iOS device. The application releases the idevicerestore_client instance, but a remove event arrives later, causing the application to crash. The crash occurs at thestrcmp(event->udid, client->udid)line in the callback function because the client instance has already been released.

Context

Here is the relevant part of the callback function that handles the remove event:

static void idevice_event_cb(const idevice_event_t* event, void* userdata)
{
    struct idevicerestore_client_t* client = (struct idevicerestore_client_t*)userdata;

    if (event->udid && !strcmp(event->udid, client->udid))
    {
        mutex_lock(&client->device_event_mutex);
        client->mode = MODE_UNKNOWN;
        debug("%s: device %016" PRIx64 " (udid: %s) disconnected\n", __func__, client->ecid, client->udid);
        client->ignore_device_add_events = 0;
        cond_signal(&client->device_event_cond);
        mutex_unlock(&client->device_event_mutex);
    }
}

Current Approach

I attempted to manage the client release by waiting for the ignore_device_add_events flag to reset using the following code, but it did not work as expected:

static void WaitForFlagReset(struct idevicerestore_client_t* client)
{
    mutex_lock(&client->device_event_mutex);
    while (client->ignore_device_add_events)
    {
        LOG_DEBUG("waiting flag ignore_device_add_events reset");
        cond_wait_timeout(&client->device_event_cond, &client->device_event_mutex, 500);
    }
    mutex_unlock(&client->device_event_mutex);
}

Full C++ Code Example

Here is the full C++ code that demonstrates how the client instance is managed and released:

bool RestoreDevice(const uint64_t ecid, const std::filesystem::path& cacheDir,
                                      const std::string& fileName, Callback callback)
{
    struct idevicerestore_client_t* client = idevicerestore_client_new();
    if (!client)
    {
        printf("Failed to create idevicerestore client\n");
        return false;
    }

    auto ideviceRestoreClient = std::unique_ptr<struct idevicerestore_client_t, ClientDeleter>(client);
    ideviceRestoreClient->flags &= ~FLAG_INTERACTIVE;
    ideviceRestoreClient->flags |= FLAG_DEBUG;
    ideviceRestoreClient->ipsw = ipsw_open(fileName.c_str());
    ideviceRestoreClient->cache_dir = strdup(cacheDir.string().c_str());

    ideviceRestoreClient->ecid = ecid;
    idevicerestore_set_progress_callback(ideviceRestoreClient.get(), Callback , &(ideviceRestoreClient->ecid));

    int result = -1;
    CURL* curl = curl_easy_init();
    if (curl)
    {
        irecv_init();
        result = idevicerestore_start(ideviceRestoreClient.get());
        curl_easy_cleanup(curl);
    }

    if (result == 0)
    {
        WaitForFlagReset(ideviceRestoreClient.get());
    }
    // unique_ptr will automatically free the client when it goes out of scope
    return result == 0;
}

I would appreciate any suggestions or insights on how to properly manage the idevicerestore client release in a multi-threaded application. Specifically, I'm looking for guidance on ensuring that the remove event is handled correctly before releasing the client instance to prevent crashes.

[Leon-cam]

Just updated my local environment for this issue.

Environment:

• Windows 11 PC
• 2 iPads used for testing

In some instances during testing, the REMOVE_EVENT did not arrive for a device that was successfully restored. Current solution might be causing an indefinite loop since the flag is not being reset properly. It might need another flag for this case.

[Leon-cam]

I recommend adding a flag, such as int restore_started to the idevicerestore_client_t structure. This flag would indicate whether the restoration process is ongoing and would be reset in the callback upon receiving a REMOVE event. This approach allows the upper caller to check the flag within its thread before releasing the client instance, ensuring safe and proper cleanup. The previous use of ignore_device_add_events is not suitable for this case.

[Leon-cam]

Hello Nikias Bassen,

Could you please review the following content? I would greatly appreciate any insights or feedback you could provide.

Regarding the implementation in restore.c. There are two static variables declared:

static int restore_finished = 0;
static int restore_device_connected = 0;

It appears that the static variable restore_device_connected is not utilized within the scope of the project. It may be an unnecessary artifact?

Additionally, it would be more efficient and maintainable to refactor the restore_finished variable. Instead of being a static variable, it would be better to integrate it into the idevicerestore_client_t C struct as a member variable.

Best regards,
Leon

[AiXanadu]

I also hope there is a version that supports multi-threaded concurrency

[AiXanadu]

#630

This problem may be similar to what you said