On 2015-09-04 12:25, Giorgio Marzano wrote:
Right now, I noticed a bug in the mares_iconhd_get_model function, line 120 of mares_iconhd.c, where sizeof should be actually strlen.
if (memcmp (device->version + 0x46, models[i].name, /*sizeof*/ strlen (models[i].name) - 1) == 0) {
This is not a bug. The name field is a 16 byte array, not a string. The initialization with a sting literal is convenient. The remainder of the array will automatically get padded with zero bytes, which is exactly what we need.
The correct fix is to add a new entry containing "Mares Smart". Then, the next question is: what's the correct model number? According to the model number in the memory dump (e.g. byte at offset 0), it's 0x10 or SMART:
{"Smart", SMART}, + {"Smart Apnea", SMART}, {"Icon HD", ICONHD},
But then how do we distinguish the normal smart from the apnea variant? That will be important, because both appear to use a different data format. The strange thing here is that the normal smart already had a freedive mode. The main difference is that we now not only get a summary of each freedive in the session, but also a detailed profile.
I wonder if there is some kind of format indicator in the header that we can use to distinguish between dives with the enhanced apnea format and the more limited normal format.
Jef
Hi, Jef
I have compiled this sniplet:
main (int argc, char *argv[]) { int i; const mares_iconhd_model_t models[] = { {"Matrix", MATRIX}, // {"Smart Apnea", SMARTAPNEA}, {"Smart", SMART}, {"Icon HD", ICONHD}, {"Icon AIR", ICONHDNET}, {"Puck Pro", PUCKPRO}, {"Nemo Wide 2", NEMOWIDE2}, {"Puck 2", PUCK2}, };
// Check the product name in the version packet against the list // with valid names, and return the corresponding model number. unsigned int model = 0; for ( i = 0; i < 7; ++i) { printf ("i: %d, sizeof: %d, strlen: %d\n",i,sizeof(models[i].name),strlen (models[i].name));
} }
and the corresponding ouput is:
giorgio@giorgio-laptop:~$ ./prova i: 0, sizeof: 17, strlen: 6 i: 1, sizeof: 17, strlen: 5 i: 2, sizeof: 17, strlen: 7 i: 3, sizeof: 17, strlen: 8 i: 4, sizeof: 17, strlen: 8 i: 5, sizeof: 17, strlen: 11 i: 6, sizeof: 17, strlen: 6
So it seems to me that either we use strlen or we use strcmp
G
2015-09-09 13:41 GMT+02:00 Jef Driesen jef@libdivecomputer.org:
On 2015-09-04 12:25, Giorgio Marzano wrote:
Right now, I noticed a bug in the mares_iconhd_get_model function, line 120 of mares_iconhd.c, where sizeof should be actually strlen.
if (memcmp (device->version + 0x46, models[i].name, /*sizeof*/ strlen (models[i].name) - 1) == 0) {
This is not a bug. The name field is a 16 byte array, not a string. The initialization with a sting literal is convenient. The remainder of the array will automatically get padded with zero bytes, which is exactly what we need.
The correct fix is to add a new entry containing "Mares Smart". Then, the next question is: what's the correct model number? According to the model number in the memory dump (e.g. byte at offset 0), it's 0x10 or SMART:
{"Smart", SMART},
{"Smart Apnea", SMART}, {"Icon HD", ICONHD},But then how do we distinguish the normal smart from the apnea variant? That will be important, because both appear to use a different data format. The strange thing here is that the normal smart already had a freedive mode. The main difference is that we now not only get a summary of each freedive in the session, but also a detailed profile.
I wonder if there is some kind of format indicator in the header that we can use to distinguish between dives with the enhanced apnea format and the more limited normal format.
Jef
On 2015-09-09 14:04, Giorgio Marzano wrote:
I have compiled this sniplet:
main (int argc, char *argv[]) { int i; const mares_iconhd_model_t models[] = { {"Matrix", MATRIX}, // {"Smart Apnea", SMARTAPNEA}, {"Smart", SMART}, {"Icon HD", ICONHD}, {"Icon AIR", ICONHDNET}, {"Puck Pro", PUCKPRO}, {"Nemo Wide 2", NEMOWIDE2}, {"Puck 2", PUCK2}, };
// Check the product name in the version packet against the list // with valid names, and return the corresponding model number. unsigned int model = 0; for ( i = 0; i < 7; ++i) { printf ("i: %d, sizeof: %d, strlen: %d\n",i,sizeof(models[i].name),strlen (models[i].name));
} }
and the corresponding ouput is:
giorgio@giorgio-laptop:~$ ./prova i: 0, sizeof: 17, strlen: 6 i: 1, sizeof: 17, strlen: 5 i: 2, sizeof: 17, strlen: 7 i: 3, sizeof: 17, strlen: 8 i: 4, sizeof: 17, strlen: 8 i: 5, sizeof: 17, strlen: 11 i: 6, sizeof: 17, strlen: 6
So it seems to me that either we use strlen or we use strcmp
No, the memcmp is correct. In the mares header, the name is a 16 byte array. If the name is shorter than 16 bytes, the remaining bytes are padded with zero's. But if there would be a name that's exactly 16 bytes long, then there won't be a terminating zero byte. In that case, strlen and strcmp will result in a buffer overflow! That's why we use memcmp. The other reason is that I want to do an exact match. The "Smart" entry should not match "Smart Apnea". Future models needs to be added explicitly, and not detected by accident.
Jef
I see your points, you are definitely right :)
G
Il 09/Set/2015 14:45, "Jef Driesen" jef@libdivecomputer.org ha scritto:
On 2015-09-09 14:04, Giorgio Marzano wrote:
I have compiled this sniplet:
main (int argc, char *argv[]) { int i; const mares_iconhd_model_t models[] = { {"Matrix", MATRIX}, // {"Smart Apnea", SMARTAPNEA}, {"Smart", SMART}, {"Icon HD", ICONHD}, {"Icon AIR", ICONHDNET}, {"Puck Pro", PUCKPRO}, {"Nemo Wide 2", NEMOWIDE2}, {"Puck 2", PUCK2}, };
// Check the product name in the version packet against the list // with valid names, and return the corresponding model number. unsigned int model = 0; for ( i = 0; i < 7; ++i) { printf ("i: %d, sizeof: %d, strlen: %d\n",i,sizeof(models[i].name),strlen (models[i].name));
} }
and the corresponding ouput is:
giorgio@giorgio-laptop:~$ ./prova i: 0, sizeof: 17, strlen: 6 i: 1, sizeof: 17, strlen: 5 i: 2, sizeof: 17, strlen: 7 i: 3, sizeof: 17, strlen: 8 i: 4, sizeof: 17, strlen: 8 i: 5, sizeof: 17, strlen: 11 i: 6, sizeof: 17, strlen: 6
So it seems to me that either we use strlen or we use strcmp
No, the memcmp is correct. In the mares header, the name is a 16 byte array. If the name is shorter than 16 bytes, the remaining bytes are padded with zero's. But if there would be a name that's exactly 16 bytes long, then there won't be a terminating zero byte. In that case, strlen and strcmp will result in a buffer overflow! That's why we use memcmp. The other reason is that I want to do an exact match. The "Smart" entry should not match "Smart Apnea". Future models needs to be added explicitly, and not detected by accident.
Jef
I don't have access to any SMART memory dump, so I can't figure how to detect the variant.
I had only two ideas, but both of them are dirty hacks: - detect using the string compare in the mares_iconhd_get_model and set some flag - develop a separate backend
G
2015-09-09 13:41 GMT+02:00 Jef Driesen jef@libdivecomputer.org:
On 2015-09-04 12:25, Giorgio Marzano wrote:
Right now, I noticed a bug in the mares_iconhd_get_model function, line 120 of mares_iconhd.c, where sizeof should be actually strlen.
if (memcmp (device->version + 0x46, models[i].name, /*sizeof*/ strlen (models[i].name) - 1) == 0) {
This is not a bug. The name field is a 16 byte array, not a string. The initialization with a sting literal is convenient. The remainder of the array will automatically get padded with zero bytes, which is exactly what we need.
The correct fix is to add a new entry containing "Mares Smart". Then, the next question is: what's the correct model number? According to the model number in the memory dump (e.g. byte at offset 0), it's 0x10 or SMART:
{"Smart", SMART},
{"Smart Apnea", SMART}, {"Icon HD", ICONHD},But then how do we distinguish the normal smart from the apnea variant? That will be important, because both appear to use a different data format. The strange thing here is that the normal smart already had a freedive mode. The main difference is that we now not only get a summary of each freedive in the session, but also a detailed profile.
I wonder if there is some kind of format indicator in the header that we can use to distinguish between dives with the enhanced apnea format and the more limited normal format.
Jef
By the way, what do the "fingerprint" field is ?
G
2015-09-09 14:18 GMT+02:00 Giorgio Marzano marzano.giorgio@gmail.com:
I don't have access to any SMART memory dump, so I can't figure how to detect the variant.
I had only two ideas, but both of them are dirty hacks:
- detect using the string compare in the mares_iconhd_get_model and set
some flag
- develop a separate backend
G
2015-09-09 13:41 GMT+02:00 Jef Driesen jef@libdivecomputer.org:
On 2015-09-04 12:25, Giorgio Marzano wrote:
Right now, I noticed a bug in the mares_iconhd_get_model function, line 120 of mares_iconhd.c, where sizeof should be actually strlen.
if (memcmp (device->version + 0x46, models[i].name, /*sizeof*/ strlen (models[i].name) - 1) == 0) {
This is not a bug. The name field is a 16 byte array, not a string. The initialization with a sting literal is convenient. The remainder of the array will automatically get padded with zero bytes, which is exactly what we need.
The correct fix is to add a new entry containing "Mares Smart". Then, the next question is: what's the correct model number? According to the model number in the memory dump (e.g. byte at offset 0), it's 0x10 or SMART:
{"Smart", SMART},
{"Smart Apnea", SMART}, {"Icon HD", ICONHD},But then how do we distinguish the normal smart from the apnea variant? That will be important, because both appear to use a different data format. The strange thing here is that the normal smart already had a freedive mode. The main difference is that we now not only get a summary of each freedive in the session, but also a detailed profile.
I wonder if there is some kind of format indicator in the header that we can use to distinguish between dives with the enhanced apnea format and the more limited normal format.
Jef
On 2015-09-09 14:22, Giorgio Marzano wrote:
By the way, what do the "fingerprint" field is ?
That's a piece of data that uniquely identifies each dive from the same dive computer. We use this for the download only new dives feature: After each download, the application is supposed to store the fingerprint of the most recent dive. One the next download, the application gives us this fingerprint, and we automatically abort the download once we detect the previously downloaded dive by means of its fingerprint.
Typically the fingerprint is simply the raw date/time bytes. But it can be something different too.
Jef
On 2015-09-09 14:18, Giorgio Marzano wrote:
I don't have access to any SMART memory dump, so I can't figure how to detect the variant.
I'll send you data from some mares smart's.
BTW, when working with memory dumps, you can use the libdivecomputer simulator. Setup instructions are available here:
http://libdivecomputer.org/simulator.html
This allows you to test the code without needing physical access to a dive computer. Very convenient.
I had only two ideas, but both of them are dirty hacks:
- detect using the string compare in the mares_iconhd_get_model and set
some flag
That's plan B. The problem here is that we identify devices by means of the model number. But we already know both the normal and apnea variant have the same model number 0x10. So we'll need to invent some new model number for the apnea variant. But if a future mares device also uses the same number, then we're in trouble. So if we can detect the data format based on the data itself, then that would be a better alternative.
- develop a separate backend
Bad idea. Devices using the same communication protocol should use the same backend. We don't want duplicated code.
Jef
G
2015-09-09 13:41 GMT+02:00 Jef Driesen jef@libdivecomputer.org:
On 2015-09-04 12:25, Giorgio Marzano wrote:
Right now, I noticed a bug in the mares_iconhd_get_model function, line 120 of mares_iconhd.c, where sizeof should be actually strlen.
if (memcmp (device->version + 0x46, models[i].name, /*sizeof*/ strlen (models[i].name) - 1) == 0) {
This is not a bug. The name field is a 16 byte array, not a string. The initialization with a sting literal is convenient. The remainder of the array will automatically get padded with zero bytes, which is exactly what we need.
The correct fix is to add a new entry containing "Mares Smart". Then, the next question is: what's the correct model number? According to the model number in the memory dump (e.g. byte at offset 0), it's 0x10 or SMART:
{"Smart", SMART},
{"Smart Apnea", SMART}, {"Icon HD", ICONHD},But then how do we distinguish the normal smart from the apnea variant? That will be important, because both appear to use a different data format. The strange thing here is that the normal smart already had a freedive mode. The main difference is that we now not only get a summary of each freedive in the session, but also a detailed profile.
I wonder if there is some kind of format indicator in the header that we can use to distinguish between dives with the enhanced apnea format and the more limited normal format.
Jef
-
Giorgio Marzano -
Jef Driesen