Lite board fails to boot if too many totp codes are configured. #384
Comments
|
I tested this on my watch with 5 entries. I suppose I was just under the limit. :( Could it be running out of memory? In order to achieve that There are some improvements I can make to both faces to immediately reduce memory usage: decoding the string every time the face activates instead of decoding them all at once during initialization. Can you help me test these fixes and confirm whether it resolves the issue? The other alternative is to go back to storing the raw decoded byte arrays in the source code which in my opinion made the face very hard to use since web sites don't output that format. @maxz has submitted a Python script to generate those arrays from a separate file at compile time, we should probably make that a priority. |
|
Not sure really how to even work out what is failing on the physical watch as the emulator works fine. I was also unable to get the lfs face to work on the physical watch too, could be related? In that case it would load the first two but the rest would not appear (possible i've done something wrong there though). More than happy to test your changes if you have some ideas already on how to reduce the memory usage. I definitely prefer not having to input the raw decodes bytes, but obviously if that works better/allow more credentials than happy to go back to that method. |
Thanks!! I'm developing a hot patch right now, I'll post the branch here once it's compiling.
Yes. I used the same algorithm as the TOTP LFS face. That made it require more RAM.
That's weird. I expected the limitations to be around the same but TOTP LFS supports even less codes? I didn't touch the TOTP LFS face in the latest commits either, so I conclude this problem must have been there for a while now. If the hot patch fixes the issue for you, I'll apply it to the LFS version as well.
Wish C had better compile time code generation support. Macros are extremely crude tools. Something like Zig's Guess we'll have to make do with a Python script.
QEMU would have been really useful right now. I need to figure out how to port the Sensor Watch board to it. |
The TOTP face is working in the simulator but fails on the real hardware when loaded with lots of codes, just like the LFS version. This is likely caused by the recent refactoring of the TOTP face which introduced a declarative credential interface for ease of use. That's accomplished by decoding the secrets at runtime which increases the RAM requirements. Users are likely hitting memory limits. In order to mitigate this, the algorithm is changed from decoding all of the secrets only once during initialization to on the fly decoding of the secret for the current TOTP credential only. This converts this face's dynamic memory usage from O(N) to O(1) at the cost of memory management when switching faces and credentials which could impact power consumption. Due to variable key sizes, the memory cannot be statically allocated. Perhaps there's a maximum key size that can serve as worst case? Also took this opportunity to restructure the code a bit. Reported-by: madhogs <[email protected]> Fixed-by: Matheus Afonso Martins Moreira <[email protected]> Tested-by: Matheus Afonso Martins Moreira <[email protected]> Tested-on-hardware-by: madhogs <[email protected]> Signed-off-by: Matheus Afonso Martins Moreira <[email protected]> Fixes: joeycastillo#384
|
Okay, I developed a hot patch which refactors the face's memory allocation. https://github.com/matheusmoreira/sensor-watch/tree/totp-hot-patch Please verify if it solves the problem. I'll submit a pull request immediately if it does. |
|
That branch seems to fix the issue 👍 , was able to load the example codes from the above branch no problem. I've only done a quick test as its quite late here, only checked it loaded, happy to test it more + check the values tomorrow. Thanks for looking into this so quickly. |
|
Great! Thanks for testing! |
The TOTP face is working in the simulator but fails on the real hardware when loaded with lots of codes, just like the LFS version. This is likely caused by the recent refactoring of the TOTP face which introduced a declarative credential interface for ease of use. That's accomplished by decoding the secrets at runtime which increases the RAM requirements. Users are likely hitting memory limits. In order to mitigate this, the algorithm is changed from decoding all of the secrets only once during initialization to on the fly decoding of the secret for the current TOTP credential only. This converts this face's dynamic memory usage from O(N) to O(1) at the cost of memory management when switching faces and credentials which could impact power consumption. Due to variable key sizes, the memory cannot be statically allocated. Perhaps there's a maximum key size that can serve as worst case? Also took this opportunity to restructure the code a bit. Also added code to check for memory allocation failure. Reported-by: madhogs <[email protected]> Fixed-by: Matheus Afonso Martins Moreira <[email protected]> Tested-by: Matheus Afonso Martins Moreira <[email protected]> Tested-on-hardware-by: madhogs <[email protected]> Signed-off-by: Matheus Afonso Martins Moreira <[email protected]> Fixes: joeycastillo#384
|
Oh boy. Unless you find another solution to the possible repeated runtime allocation (on a quick glance it at least does not seem to call malloc for the same entry twice but to use memset) and decoding I guess I would prefer for the solution in the main branch to be the one from my PR #356 which has to deal with the explicit key array but has very little runtime overhead. @joeycastillo also spoke out against runtime heap allocation within faces in the past (But I can't find the PR right now, it was some PR by @theAlexes so they might remember which one it was.) |
|
@maxz I have eliminated the dynamic memory allocation. Now there is a single user overridable 128 byte buffer. Only the runtime decoding overhead remains which will also be eliminated when the Python script is integrated.
Me too... But we are programmers. Users might not be comfortable with munging bytes and editing them into source code. The watch face's documentation also suggests using a web service to decode the bytes which could leak the secret. I really wanted to avoid the need for that.
Yes, in the discord:
I agree with him and have therefore removed the dynamic memory allocation. |
The TOTP face is working in the simulator but fails on the real hardware when loaded with lots of codes, just like the LFS version. This is likely caused by the recent refactoring of the TOTP face which introduced a declarative credential interface for ease of use. That's accomplished by decoding the secrets at runtime which increases the RAM requirements. Users are likely hitting memory limits. In order to mitigate this, the algorithm is changed from decoding all of the secrets only once during initialization to on the fly decoding of the secret for the current TOTP credential only. This converts this face's dynamic memory usage from O(N) to O(1) at the cost of memory management when switching faces and credentials which could impact power consumption. Issue is confirmed fixed by author of issue who has tested it on real hardware. Fixes joeycastillo#384. Due to variable key sizes, the memory cannot be statically allocated. Perhaps there's a maximum key size that can serve as worst case? Also took this opportunity to restructure the code a bit. Also added code to check for memory allocation failure. Reported-by: madhogs <[email protected]> Fixed-by: Matheus Afonso Martins Moreira <[email protected]> Tested-by: Matheus Afonso Martins Moreira <[email protected]> Tested-on-hardware-by: madhogs <[email protected]> Signed-off-by: Matheus Afonso Martins Moreira <[email protected]> GitHub-Issue: joeycastillo#384
The TOTP face is working in the simulator but fails on the real hardware when loaded with lots of codes, just like the LFS version. This is likely caused by the recent refactoring of the TOTP face which introduced a declarative credential interface for ease of use. That's accomplished by decoding the secrets at runtime which increases the RAM requirements. Users are likely hitting memory limits. In order to mitigate this, the algorithm is changed from decoding all of the secrets only once during initialization to on the fly decoding of the secret for the current TOTP credential only. This converts this face's dynamic memory usage from O(N) to O(1) at the cost of memory management when switching faces and credentials which could impact power consumption. Issue is confirmed fixed by author of issue who has tested it on real hardware. Fixes #384. Due to variable key sizes, the memory cannot be statically allocated. Perhaps there's a maximum key size that can serve as worst case? Also took this opportunity to restructure the code a bit. Also added code to check for memory allocation failure. Reported-by: madhogs <[email protected]> Fixed-by: Matheus Afonso Martins Moreira <[email protected]> Tested-by: Matheus Afonso Martins Moreira <[email protected]> Tested-on-hardware-by: madhogs <[email protected]> Signed-off-by: Matheus Afonso Martins Moreira <[email protected]> GitHub-Issue: #384
If, on the totp_face, I add more than just a few totp codes the watch fails to boot and doesn't display anything. I think this is to do with the new credentials configuration as I have the same credentials working on my other (also sensor watch lite) watch which is still on the old firmware.
I have created a branch here - main...madhogs:Sensor-Watch:example-too-many-totp
The above has some example credentials that are the same length and number as the real ones i have. After building the above with 'make color=RED' and flashing the watch it no longer displays anything when the battery is connected. Reducing the list to 5 or less entries and it works again.
It always works when running in the emulator, it only fails on the physical watch, i wonder if it is some hardware limitation when converting these codes.
The text was updated successfully, but these errors were encountered: