Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in param TOC CRC calculation #192

Closed
ataffanel opened this issue Jan 24, 2017 · 2 comments
Closed

Bug in param TOC CRC calculation #192

ataffanel opened this issue Jan 24, 2017 · 2 comments
Milestone

Comments

@ataffanel
Copy link
Member

There seems to be a bug with the param TOC CRC calculation. The bug was discovered in this ticket in the client bitcraze/crazyflie-clients-python#281. The CRC is used by the client to cache the TOC and the CRC calculation seems unreliable. Running some test I have observed 2 problematic behaviors:

  • Changing a parameter does not necessarly change the param TOC crc
  • Changing the LOG toc can result in changing the param toc even if the param toc has not been changed (!)

In the process the log TOC CRC should also be checked.

@ataffanel
Copy link
Member Author

Found a fondamental problems on how the toc are hashed!

The code that calculate the CRC is:

  params = &_param_start;
  paramsLen = &_param_stop - &_param_start;
  paramsCrc = crcSlow(params, paramsLen*sizeof(params[0]));

Looks simple enough, it does CRC the memory area that contains the full param toc. The problem is that when using objdump to look at this memory area this is what we see:

0801dcec <_param_start>:
(...)
0801e244 <__params_ring>:
 801e244:       00000081 0801b578 00000000 00000008     ....x...........
 801e254:       0801c6fc 20000620 0000004a 0801c6fb     .... .. J.......
 801e264:       2000a89c 00000008 0801d314 20000628     ... ........(.. 
 801e274:       00000008 0801d31d 2000061a 00000008     ........... ....
 801e284:       0801d328 20000629 00000008 0801d332     (...).. ....2...
 801e294:       2000a894 00000006 0801d342 2000062c     ... ....B...,.. 
 801e2a4:       00000006 0801d34b 2000061c 00000006     ....K...... ....
 801e2b4:       0801d357 20000630 00000080 0801b573     W...0.. ....s...
 801e2c4:       00000000                                ....

There is no strings at all! The toc contains structs that have pointer to strings and not the string content. The strings are in the object file that has declared the params, somewhere else in memory:

0801d2dc <colorRing>:
 801d2dc:       00200000 00001000 04000008 08101010     .. .............
 801d2ec:       04040808 00002004 08000010 00040000     ..... ..........
 801d2fc:       00000200 676e6972 656d6954 63620072     ....ringTimer.bc
 801d30c:       5264654c 00676e69 696c6f73 64655264     LedRing.solidRed
 801d31c:       6c6f7300 72476469 006e6565 696c6f73     .solidGreen.soli
 801d32c:       756c4264 65680065 696c6461 45746867     dBlue.headlightE
 801d33c:       6c62616e 6c670065 7473776f 65007065     nable.glowstep.e
 801d34c:       7974706d 72616843 66006567 436c6c75     mptyCharge.fullC
 801d35c:       67726168 00000065    

So, doing a checksum of the memory area containing the TOC is wrong: it only tests part of the TOC, basically the sequence of params and their types, but not the names.

One solution is to hash the descriptions of each entry. The hash should represent all info that are sent to the ground, ideally no more but definitely no less!

@ataffanel
Copy link
Member Author

Fixed by using types and strings content in CRC calculation. Now if two TOC have the same hash, there is a big probability that it is the same TOC.
Unfortunately the hash is still related to the TOC element order in memory which means that a different compiler, compiler settings, or Makefile, might generate a different hash for the same TOC. Short of using a lot of memory to sort the TOC there is not much we can do about it and since this is a connection time optimisation feature some false negative do not matter.

@ataffanel ataffanel added this to the Next version milestone Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant