Fix nfc_protocol_support_scene_save_name_on_event crash

[xMasterX]

What's new

• Fixed crash with wrong (leftover from previous reading) protocols_detected list being used in Add manually Scene (in save_name -> on event call) after reading mifare classic tag and going to add manually with selecting other protocol

Verification

• Open NFC app
• Select Read -> Apply any mifare classic tag -> After read press back, Select Cancel
• Select Add manually -> NFC-A 4-bytes UID
• Don't enter any data or enter any data - doesnt matter. click save until it reaches file name
• Click save, oh, we are crashed
• Verify same steps with PR applied, all should work without any crashes

Checklist (For Reviewer)

• PR has description of feature/bug or link to Confluence/Jira task
• Description contains actions to verify feature/bugfix
• I've built this code, uploaded it to the device and verified feature/bugfix

[gornekich]

Hi @xMasterX , thanks for PR!

The problem behind this issue is in nfc_protocol_support_scene_save_name_on_event() function. We assume that we come to this scene after reading and get protocol from list of detected protocols, however we may come to this scene from "Add manually". If MfClassic protocol was detected before, it leads to call of nfc_scene_save_name_on_event_mf_classic() function and flipper crashes on nfc_get_data() call with incorrect protocol.

Your fix works because there is no "save_name_*()" functions for Iso14443a protocols. However it may lead to another issues if we implement these function in future.

The correct fix is to get protocol from nfc_device in nfc_protocol_support_scene_save_name_on_event() function. Please, consider the patch with these fixes and if everything is OK, apply it and we will merge your PR.
fix_save_name.txt

[xMasterX]

Hi @xMasterX , thanks for PR!

The problem behind this issue is in nfc_protocol_support_scene_save_name_on_event() function. We assume that we come to this scene after reading and get protocol from list of detected protocols, however we may come to this scene from "Add manually". If MfClassic protocol was detected before, it leads to call of nfc_scene_save_name_on_event_mf_classic() function and flipper crashes on nfc_get_data() call with incorrect protocol.

Your fix works because there is no "save_name_*()" functions for Iso14443a protocols. However it may lead to another issues if we implement these function in future.

The correct fix is to get protocol from nfc_device in nfc_protocol_support_scene_save_name_on_event() function. Please, consider the patch with these fixes and if everything is OK, apply it and we will merge your PR. fix_save_name.txt

Hi,

For some reason part of the functions in applications/main/nfc/helpers/protocol_support/nfc_protocol_support.c
are using nfc_device_get_protocol and other part is using instance->protocols_detected[instance->protocols_detected_selected_idx];
Thats why I preferred to not touch this part

And yes if NfcProtocolIso14443_3a gets _save_name_on_event function it will be an issue

About your patch and Not clearing the array at start scene, why should we keep array filled with non actual data (originally its filled with 0s at start of the NFC app, which is == NfcProtocolIso14443_3a = 0)?
When we exit from Read array will stay filled with old data
With patch applied It won't be used in nfc_protocol_support_scene_save_name_on_event but may lead to issues in future if we use data from that array in some other place (after we exit from Read)
As for using nfc_device_get_protocol this seems correct solution but was avoided by me in first place due to lack of understanding of logical reasons why we are using one and other way to get protocol in /nfc_protocol_support.c (only in nfc_protocol_support_scene_read_on_event it seems correct for specific events)

[gornekich]

For some reason part of the functions in applications/main/nfc/helpers/protocol_support/nfc_protocol_support.c
are using nfc_device_get_protocol and other part is using instance->protocols_detected[instance->protocols_detected_selected_idx];

instance->protocols_detected[instance->protocols_detected_selected_idx]; must be used only during reading. Once we set data to NfcDevice instance, we should use nfc_device_get_protocol() function.

About your patch and Not clearing the array at start scene, why should we keep array filled with non actual data (originally its filled with 0s at start of the NFC app, which is == NfcProtocolIso14443_3a = 0)?
When we exit from Read array will stay filled with old data
With patch applied It won't be used in nfc_protocol_support_scene_save_name_on_event but may lead to issues in future if we use data from that array in some other place (after we exit from Read)

I just didn't like setting array of detected protocol to NfcProtocolIso14443_3a explicitly from start scene. What if we change value of NfcProtocolIso14443_3a to another value by just changing order in enumeration. The correct way to deal with detected protocols is to make a private class with getters and setters. And this class will correctly reset it state internally.

Thanks for noticing that. I think we should merge this PR for now, since it fixes the issue, and we will refactor working with detected protocol in future.