Re-do the libdivecomputer fingerprint save/load code

This tries to make our fingerprinting code work better, by avoiding
using the "deviceid" field that has always been unreliable because we've
calculated it multiple different ways, and even for the same version of
subsurface, it ends up changing in the middle (ie we calculate one value
initially, then re-calculate it when we have a proper serial number
string).

So instead, the fingerprinting code will look up and save the
fingerprint file using purely "stable" information that is available
early during the download:

 - the device model name (which is a string with vendor and product name
   separated by a space)

 - the DC_EVENT_DEVINFO 32-bit 'serial' number (which is not necessarily
   a real serial number at all, but hopefully at least a unique number
   for the particular product)

but because the model name is not necessarily a good filename (think
slashes and other possibly invalid characters), we hash that model name
and use the resulting hex number in the fingerprint file name.

This way the fingerprint file is unambiguous at load and save time, and
depends purely on libdivecomputer data.

But because we also need to verify that we have the actual _dive_
associated with that fingerprint, we also need to save the final
deviceid and diveid when saving the fingerprint file, so that when we
load it again we can look up the dive and verify that we have it before
we use the fingerprint data.

To do that, the fingerprint file itself contains not just the
fingerprint data from libdivecomputer, but the last 8 bytes of the file
are the (subsurface) deviceid and the diveid of the dive that is
associated with the fingerprint.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

8 files changed, 103 insertions, 171 deletions

diff --git a/core/downloadfromdcthread.cpp b/core/downloadfromdcthread.cpp
index 554618388..6c38812c9 100644
--- a/core/downloadfromdcthread.cpp
+++ b/core/downloadfromdcthread.cpp
@@ -218,7 +218,6 @@ DCDeviceData::DCDeviceData()
 	memset(&data, 0, sizeof(data));
 	data.download_table = nullptr;
 	data.diveid = 0;
-	data.deviceid = 0;
 #if defined(BT_SUPPORT)
 	data.bluetooth_mode = true;
 #else
@@ -293,11 +292,6 @@ bool DCDeviceData::forceDownload() const
 	return data.force_download;
 }
 
-int DCDeviceData::deviceId() const
-{
-	return data.deviceid;
-}
-
 int DCDeviceData::diveId() const
 {
 	return data.diveid;
@@ -357,11 +351,6 @@ void DCDeviceData::setForceDownload(bool force)
 	data.force_download = force;
 }
 
-void DCDeviceData::setDeviceId(int deviceId)
-{
-	data.deviceid = deviceId;
-}
-
 void DCDeviceData::setDiveId(int diveId)
 {
 	data.diveid = diveId;
diff --git a/core/downloadfromdcthread.h b/core/downloadfromdcthread.h
index 448271211..a4a485a0e 100644
--- a/core/downloadfromdcthread.h
+++ b/core/downloadfromdcthread.h
@@ -32,7 +32,6 @@ public:
 	QString descriptor() const;
 	bool forceDownload() const;
 	bool saveLog() const;
-	int deviceId() const;
 	int diveId() const;
 
 	/* this needs to be a pointer to make the C-API happy */
@@ -44,7 +43,6 @@ public:
 	int getDetectedVendorIndex();
 	int getDetectedProductIndex(const QString &currentVendorText);
 
-	void setDeviceId(int deviceId);
 	void setDiveId(int diveId);
 	void setVendor(const QString& vendor);
 	void setProduct(const QString& product);
diff --git a/core/libdivecomputer.c b/core/libdivecomputer.c
index c750efdd6..0d797b66a 100644
--- a/core/libdivecomputer.c
+++ b/core/libdivecomputer.c
@@ -600,7 +600,8 @@ static dc_status_t libdc_header_parser(dc_parser_t *parser, device_data_t *devda
 		return rc;
 	}
 
-	dive->dc.deviceid = devdata->deviceid;
+	// Our deviceid is the hash of the serial number
+	dive->dc.deviceid = 0;
 
 	if (rc == DC_STATUS_SUCCESS) {
 		tm.tm_year = dt.year;
@@ -771,16 +772,6 @@ static int dive_cb(const unsigned char *data, unsigned int size,
 	dive->dc.model = strdup(devdata->model);
 	dive->dc.diveid = calculate_diveid(fingerprint, fsize);
 
-	/* Should we add it to the cached fingerprint file? */
-	if (fingerprint && fsize && !devdata->fingerprint) {
-		devdata->fingerprint = calloc(fsize, 1);
-		if (devdata->fingerprint) {
-			devdata->fsize = fsize;
-			devdata->fdiveid = dive->dc.diveid;
-			memcpy(devdata->fingerprint, fingerprint, fsize);
-		}
-	}
-
 	// Parse the dive's header data
 	rc = libdc_header_parser (parser, devdata, dive);
 	if (rc != DC_STATUS_SUCCESS) {
@@ -797,6 +788,22 @@ static int dive_cb(const unsigned char *data, unsigned int size,
 
 	dc_parser_destroy(parser);
 
+	/*
+	 * Save off fingerprint data.
+	 *
+	 * NOTE! We do this after parsing the dive fully, so that
+	 * we have the final deviceid here.
+	 */
+	if (fingerprint && fsize && !devdata->fingerprint) {
+		devdata->fingerprint = calloc(fsize, 1);
+		if (devdata->fingerprint) {
+			devdata->fsize = fsize;
+			devdata->fdeviceid = dive->dc.deviceid;
+			devdata->fdiveid = dive->dc.diveid;
+			memcpy(devdata->fingerprint, fingerprint, fsize);
+		}
+	}
+
 	/* If we already saw this dive, abort. */
 	if (!devdata->force_download && find_dive(&dive->dc)) {
 		char *date_string = get_dive_date_c_string(dive->when);
@@ -822,95 +829,6 @@ error_exit:
 
 }
 
-/*
- * The device ID for libdivecomputer devices is the first 32-bit word
- * of the SHA1 hash of the model/firmware/serial numbers.
- *
- * NOTE! This is byte-order-dependent. And I can't find it in myself to
- * care.
- */
-static uint32_t calculate_sha1(unsigned int model, unsigned int firmware, unsigned int serial)
-{
-	SHA_CTX ctx;
-	uint32_t csum[5];
-
-	SHA1_Init(&ctx);
-	SHA1_Update(&ctx, &model, sizeof(model));
-	SHA1_Update(&ctx, &firmware, sizeof(firmware));
-	SHA1_Update(&ctx, &serial, sizeof(serial));
-	SHA1_Final((unsigned char *)csum, &ctx);
-	return csum[0];
-}
-
-/*
- * libdivecomputer has returned two different serial numbers for the
- * same device in different versions. First it used to just do the four
- * bytes as one 32-bit number, then it turned it into a decimal number
- * with each byte giving two digits (0-99).
- *
- * The only way we can tell is by looking at the format of the number,
- * so we'll just fix it to the first format.
- */
-static unsigned int undo_libdivecomputer_suunto_nr_changes(unsigned int serial)
-{
-	unsigned char b0, b1, b2, b3;
-
-	/*
-	 * The second format will never have more than 8 decimal
-	 * digits, so do a cheap check first
-	 */
-	if (serial >= 100000000)
-		return serial;
-
-	/* The original format seems to be four bytes of values 00-99 */
-	b0 = (serial >> 0) & 0xff;
-	b1 = (serial >> 8) & 0xff;
-	b2 = (serial >> 16) & 0xff;
-	b3 = (serial >> 24) & 0xff;
-
-	/* Looks like an old-style libdivecomputer serial number */
-	if ((b0 < 100) && (b1 < 100) && (b2 < 100) && (b3 < 100))
-		return serial;
-
-	/* Nope, it was converted. */
-	b0 = serial % 100;
-	serial /= 100;
-	b1 = serial % 100;
-	serial /= 100;
-	b2 = serial % 100;
-	serial /= 100;
-	b3 = serial % 100;
-
-	serial = b0 + (b1 << 8) + (b2 << 16) + (b3 << 24);
-	return serial;
-}
-
-static unsigned int fixup_suunto_versions(device_data_t *devdata, const dc_event_devinfo_t *devinfo)
-{
-	unsigned int serial = devinfo->serial;
-	char serial_nr[13] = "";
-	char firmware[13] = "";
-
-	first_temp_is_air = 1;
-
-	serial = undo_libdivecomputer_suunto_nr_changes(serial);
-
-	if (serial) {
-		snprintf(serial_nr, sizeof(serial_nr), "%02d%02d%02d%02d",
-			 (devinfo->serial >> 24) & 0xff,
-			 (devinfo->serial >> 16) & 0xff,
-			 (devinfo->serial >> 8) & 0xff,
-			 (devinfo->serial >> 0) & 0xff);
-	}
-	if (devinfo->firmware) {
-		snprintf(firmware, sizeof(firmware), "%d.%d.%d",
-			 (devinfo->firmware >> 16) & 0xff,
-			 (devinfo->firmware >> 8) & 0xff,
-			 (devinfo->firmware >> 0) & 0xff);
-	}
-
-	return serial;
-}
 #ifndef O_BINARY
   #define O_BINARY 0
 #endif
@@ -922,11 +840,14 @@ static void do_save_fingerprint(device_data_t *devdata, const char *tmp, const c
 	if (fd < 0)
 		return;
 
+	dev_info(devdata, "Saving fingerprint for %08x:%08x to '%s'",
+		devdata->fdeviceid, devdata->fdiveid, final);
+
 	/* The fingerprint itself.. */
 	written = write(fd, devdata->fingerprint, devdata->fsize);
 
-	/* ..followed by the dive ID of the fingerprinted dive */
-	if (write(fd, &devdata->fdiveid, 4) != 4)
+	/* ..followed by the device ID and dive ID of the fingerprinted dive */
+	if (write(fd, &devdata->fdeviceid, 4) != 4 || write(fd, &devdata->fdiveid, 4) != 4)
 		written = -1;
 
 	/* I'd like to do fsync() here too, but does Windows support it? */
@@ -934,33 +855,76 @@ static void do_save_fingerprint(device_data_t *devdata, const char *tmp, const c
 		written = -1;
 
 	if (written == devdata->fsize) {
-		if (!subsurface_rename(tmp, final))
+		if (!subsurface_rename(tmp, final)) {
+			dev_info(devdata, "  ... %d bytes and dive ID", written);
 			return;
+		}
 	}
 	unlink(tmp);
 }
 
+static char *fingerprint_file(device_data_t *devdata)
+{
+	uint32_t model, serial;
+
+	// Model hash and libdivecomputer 32-bit 'serial number' for the file name
+	model = calculate_string_hash(devdata->model);
+	serial = devdata->devinfo.serial;
+
+	return format_string("%s/fingerprints/%04x.%u",
+		system_default_directory(),
+		model, serial);
+}
+
 /*
  * Save the fingerprint after a successful download
+ *
+ * NOTE! At this point, we have the final device ID for the divecomputer
+ * we downloaded from. But that 'deviceid' is actually not useful, because
+ * at the point where we want to _load_ this, we only have the libdivecomputer
+ * DC_EVENT_DEVINFO state (devdata->devinfo).
+ *
+ * Now, we do have the devdata->devinfo at save time, but at load time we
+ * need to verify not only that it's the proper fingerprint file: we also
+ * need to check that we actually have the particular dive that was
+ * associated with that fingerprint state.
+ *
+ * That means that the fingerprint save file needs to include not only the
+ * fingerprint data itself, but also enough data to look up a dive unambiguously
+ * when loading the fingerprint. And the fingerprint data needs to be looked
+ * up using the DC_EVENT_DEVINFO data.
+ *
+ * End result:
+ *
+ *   - fingerprint filename depends on the model name and 'devinfo.serial'
+ *     so that we can look it up at DC_EVENT_DEVINFO time before the full
+ *     info has been parsed.
+ *
+ *   - the fingerprint file contains the 'diveid' of the fingerprinted dive,
+ *     which is just a hash of the fingerprint itself.
+ *
+ *   - we also save the final 'deviceid' in the fingerprint file, so that
+ *     looking up the dive associated with the fingerprint is possible.
  */
 static void save_fingerprint(device_data_t *devdata)
 {
 	char *dir, *tmp, *final;
 
-	if (!devdata->fingerprint)
+	// Don't try to save nonexistent fingerprint data
+	if (!devdata->fingerprint || !devdata->fdiveid)
 		return;
 
+	// Make sure the fingerprints directory exists
 	dir = format_string("%s/fingerprints", system_default_directory());
 	subsurface_mkdir(dir);
-	tmp = format_string("%s/%04x.tmp", dir, devdata->deviceid);
-	final = format_string("%s/%04x", dir, devdata->deviceid);
+
+	final = fingerprint_file(devdata);
+	tmp = format_string("%s.tmp", final);
 	free(dir);
 
 	do_save_fingerprint(devdata, tmp, final);
 	free(tmp);
 	free(final);
-	free(devdata->fingerprint);
-	devdata->fingerprint = NULL;
 }
 
 static int has_dive(unsigned int deviceid, unsigned int diveid)
@@ -984,26 +948,31 @@ static int has_dive(unsigned int deviceid, unsigned int diveid)
 
 /*
  * The fingerprint cache files contain the actual libdivecomputer
- * fingerprint, followed by 4 bytes of diveid data. Before we use
- * the fingerprint data, verify that we actually do have that
- * fingerprinted dive.
+ * fingerprint, followed by 8 bytes of (deviceid,diveid) data.
+ *
+ * Before we use the fingerprint data, verify that we actually
+ * do have that fingerprinted dive.
  */
 static void verify_fingerprint(dc_device_t *device, device_data_t *devdata, const unsigned char *buffer, size_t size)
 {
-	unsigned int diveid, deviceid;
+	uint32_t diveid, deviceid;
 
-	if (size <= 4)
+	if (size <= 8)
 		return;
-	size -= 4;
+	size -= 8;
 
 	/* Get the dive ID from the end of the fingerprint cache file.. */
-	memcpy(&diveid, buffer + size, 4);
-	/* .. and the device ID from the device data */
-	deviceid = devdata->deviceid;
+	memcpy(&deviceid, buffer + size, 4);
+	memcpy(&diveid, buffer + size + 4, 4);
 
+	dev_info(devdata, " ... fingerprinted dive %08x:%08x", deviceid, diveid);
 	/* Only use it if we *have* that dive! */
-	if (has_dive(deviceid, diveid))
-		dc_device_set_fingerprint(device, buffer, size);
+	if (!has_dive(deviceid, diveid)) {
+		dev_info(devdata, " ... dive not found", deviceid, diveid);
+		return;
+	}
+	dc_device_set_fingerprint(device, buffer, size);
+	dev_info(devdata, " ... fingerprint of size %zu", size);
 }
 
 /*
@@ -1018,9 +987,11 @@ static void lookup_fingerprint(dc_device_t *device, device_data_t *devdata)
 
 	if (devdata->force_download)
 		return;
-	cachename = format_string("%s/fingerprints/%04x",
-		system_default_directory(), devdata->deviceid);
+
+	cachename = fingerprint_file(devdata);
+	dev_info(devdata, "Looking for fingerprint in '%s'", cachename);
 	if (readfile(cachename, &mem) > 0) {
+		dev_info(devdata, " ... got %zu bytes", mem.size);
 		verify_fingerprint(device, devdata, mem.buffer, mem.size);
 		free(mem.buffer);
 	}
@@ -1035,7 +1006,6 @@ static void event_cb(dc_device_t *device, dc_event_type_t event, const void *dat
 	const dc_event_clock_t *clock = data;
 	const dc_event_vendor_t *vendor = data;
 	device_data_t *devdata = userdata;
-	unsigned int serial;
 
 	switch (event) {
 	case DC_EVENT_WAITING:
@@ -1070,19 +1040,7 @@ static void event_cb(dc_device_t *device, dc_event_type_t event, const void *dat
 				devinfo->serial, devinfo->serial);
 		}
 
-		/*
-		 * libdivecomputer doesn't give serial numbers in the proper string form,
-		 * so we have to see if we can do some vendor-specific munging.
-		 */
-		serial = devinfo->serial;
-		if (!strcmp(devdata->vendor, "Suunto"))
-			serial = fixup_suunto_versions(devdata, devinfo);
-		devdata->deviceid = calculate_sha1(devinfo->model, devinfo->firmware, serial);
-		/* really, firmware version is NOT a number. We'll try to save it here
-		 * in something that might work, but this really needs to be handled with the
-		 * DC_FIELD_STRING interface instead */
-		devdata->libdc_firmware = devinfo->firmware;
-
+		devdata->devinfo = *devinfo;
 		lookup_fingerprint(device, devdata);
 
 		break;
@@ -1495,6 +1453,8 @@ const char *do_libdivecomputer_import(device_data_t *data)
 	 * it refers to before we use the fingerprint data.
 	 */
 	save_fingerprint(data);
+	free(data->fingerprint);
+	data->fingerprint = NULL;
 
 	return err;
 }
diff --git a/core/libdivecomputer.h b/core/libdivecomputer.h
index 9932c9d84..21cd3f83e 100644
--- a/core/libdivecomputer.h
+++ b/core/libdivecomputer.h
@@ -35,9 +35,9 @@ typedef struct {
 	const char *vendor, *product, *devname;
 	const char *model, *btname;
 	unsigned char *fingerprint;
-	unsigned int fsize, fdiveid;
-	uint32_t libdc_firmware;
-	uint32_t deviceid, diveid;
+	unsigned int fsize, fdeviceid, fdiveid;
+	struct dc_event_devinfo_t devinfo;
+	uint32_t diveid;
 	dc_device_t *device;
 	dc_context_t *context;
 	dc_iostream_t *iostream;
diff --git a/desktop-widgets/configuredivecomputerdialog.cpp b/desktop-widgets/configuredivecomputerdialog.cpp
index 6434f8c05..e8af051a0 100644
--- a/desktop-widgets/configuredivecomputerdialog.cpp
+++ b/desktop-widgets/configuredivecomputerdialog.cpp
@@ -273,7 +273,7 @@ void OstcFirmwareCheck::checkLatest(QWidget *_parent, device_data_t *data)
 	// for the OSTC that means highbyte.lowbyte is the version number
 	// For OSTC 4's its stored as XXXX XYYY YYZZ ZZZB, -> X.Y.Z beta?
 
-	int firmwareOnDevice = devData.libdc_firmware;
+	int firmwareOnDevice = devData.devinfo.firmware;
 	QString firmwareOnDeviceString;
 	// Convert the latestFirmwareAvailable to a integear we can compare with
 	QStringList fwParts = latestFirmwareAvailable.split(".");
@@ -907,7 +907,8 @@ void ConfigureDiveComputerDialog::getDeviceData()
 	device_data.product = copy_qstring(selected_product);
 
 	device_data.descriptor = descriptorLookup.value(selected_vendor.toLower() + selected_product.toLower());
-	device_data.deviceid = device_data.diveid = 0;
+	device_data.diveid = 0;
+	memset(&device_data.devinfo, 0, sizeof(device_data.devinfo));
 
 	qPrefDiveComputer::set_device(device_data.devname);
 #ifdef BT_SUPPORT
diff --git a/mobile-widgets/qmlmanager.cpp b/mobile-widgets/qmlmanager.cpp
index 313e3fcb5..41be77f59 100644
--- a/mobile-widgets/qmlmanager.cpp
+++ b/mobile-widgets/qmlmanager.cpp
@@ -1892,16 +1892,6 @@ bool QMLManager::DC_saveDump() const
 	return DCDeviceData::instance()->saveDump();
 }
 
-int QMLManager::DC_deviceId() const
-{
-	return DCDeviceData::instance()->deviceId();
-}
-
-void QMLManager::DC_setDeviceId(int deviceId)
-{
-	DCDeviceData::instance()->setDeviceId(deviceId);
-}
-
 void QMLManager::DC_setVendor(const QString& vendor)
 {
 	DCDeviceData::instance()->setVendor(vendor);
diff --git a/mobile-widgets/qmlmanager.h b/mobile-widgets/qmlmanager.h
index 89fdce323..e43c666ec 100644
--- a/mobile-widgets/qmlmanager.h
+++ b/mobile-widgets/qmlmanager.h
@@ -48,7 +48,6 @@ class QMLManager : public QObject {
 	Q_PROPERTY(bool DC_forceDownload READ DC_forceDownload WRITE DC_setForceDownload NOTIFY DC_ForceDownloadChanged)
 	Q_PROPERTY(bool DC_bluetoothMode READ DC_bluetoothMode WRITE DC_setBluetoothMode)
 	Q_PROPERTY(bool DC_saveDump READ DC_saveDump WRITE DC_setSaveDump)
-	Q_PROPERTY(int DC_deviceId READ DC_deviceId WRITE DC_setDeviceId)
 	Q_PROPERTY(QString pluggedInDeviceName MEMBER m_pluggedInDeviceName NOTIFY pluggedInDeviceNameChanged)
 	Q_PROPERTY(bool showNonDiveComputers MEMBER m_showNonDiveComputers WRITE setShowNonDiveComputers NOTIFY showNonDiveComputersChanged)
 	Q_PROPERTY(qPrefCloudStorage::cloud_status oldStatus MEMBER m_oldStatus WRITE setOldStatus NOTIFY oldStatusChanged)
@@ -101,9 +100,6 @@ public:
 	bool DC_saveDump() const;
 	void DC_setSaveDump(bool dumpMode);
 
-	int DC_deviceId() const;
-	void DC_setDeviceId(int deviceId);
-
 	QString getUndoText() const;
 	QString getRedoText() const;
 
diff --git a/smtk-import/smartrak.c b/smtk-import/smartrak.c
index 257cc143f..9eeb7bb13 100644
--- a/smtk-import/smartrak.c
+++ b/smtk-import/smartrak.c
@@ -871,7 +871,6 @@ static int prepare_data(int data_model, char *serial, dc_family_t dc_fam, device
 	if (!data_model){
 		dev_data->model = copy_string("manually added dive");
 		dev_data->descriptor = NULL;
-		dev_data->deviceid = 0;
 		return DC_STATUS_NODEVICE;
 	}
 	dev_data->descriptor = get_data_descriptor(data_model, dc_fam);
@@ -879,11 +878,11 @@ static int prepare_data(int data_model, char *serial, dc_family_t dc_fam, device
 		dev_data->vendor = dc_descriptor_get_vendor(dev_data->descriptor);
 		dev_data->product = dc_descriptor_get_product(dev_data->descriptor);
 		dev_data->model = smtk_concat_str(dev_data->model, "", "%s %s", dev_data->vendor, dev_data->product);
-		dev_data->deviceid = (uint32_t) lrint(strtod(serial, NULL));
+		dev_data->devinfo.serial = (uint32_t) lrint(strtod(serial, NULL));
 		return DC_STATUS_SUCCESS;
 	} else {
 		dev_data->model = copy_string("unsupported dive computer");
-		dev_data->deviceid = (uint32_t) lrint(strtod(serial, NULL));
+		dev_data->devinfo.serial = (uint32_t) lrint(strtod(serial, NULL));
 		return DC_STATUS_UNSUPPORTED;
 	}
 }
@@ -1012,7 +1011,6 @@ void smartrak_import(const char *file, struct dive_table *divetable)
 				dc_fam = DC_FAMILY_UWATEC_ALADIN;
 		}
 		rc = prepare_data(dc_model, copy_string(col[coln(DCNUMBER)]->bind_ptr), dc_fam, devdata);
-		smtkdive->dc.deviceid = devdata->deviceid;
 		smtkdive->dc.model = copy_string(devdata->model);
 		if (rc == DC_STATUS_SUCCESS && *bound_lens[coln(PROFILE)]) {
 			prf_buffer = mdb_ole_read_full(mdb, col[coln(PROFILE)], &prf_length);