Skip to content
New issue

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

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

Already on GitHub? # to your account

[FL-3884] Proper integer parsing #3839

Merged
merged 19 commits into from
Sep 5, 2024
Merged

Conversation

portasynthinca3
Copy link
Member

@portasynthinca3 portasynthinca3 commented Aug 13, 2024

What's new

  • Adds the following string to integer conversion API functions that verify input validity including overflow checks (header lib/toolbox/strint.h):
    StrintParseError strint_to_uint64(const char* str, char** end, uint64_t* out, uint8_t base);
    StrintParseError strint_to_int64(const char* str, char** end, int64_t* out, uint8_t base);
    StrintParseError strint_to_uint32(const char* str, char** end, uint32_t* out, uint8_t base);
    StrintParseError strint_to_int32(const char* str, char** end, int32_t* out, uint8_t base);
    StrintParseError strint_to_uint16(const char* str, char** end, uint16_t* out, uint8_t base);
    StrintParseError strint_to_int16(const char* str, char** end, int16_t* out, uint8_t base);
  • Adds unit tests for these new functions
  • Changes existing code to use these new functions instead of sscanf and strtol and friends wherever warranted and possible

Verification

Run unit tests

Remarks

  • "strint" is not a typo, it's short for "string & integer"
  • not really sure if the API version in api_symbols.csv is correct

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

Copy link

github-actions bot commented Aug 13, 2024

PVS-Studio report for commit 2a4d8401:

Copy link
Member

@hedger hedger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sync all targets' API versions.
./fbt TARGET_HW=18

@portasynthinca3 portasynthinca3 force-pushed the portasynthinca3/3884-int-parsing branch from 383b76d to 056a691 Compare August 13, 2024 15:32
Copy link

github-actions bot commented Aug 13, 2024

Compiled f7 firmware for commit df6d9ad3:

@portasynthinca3
Copy link
Member Author

Not sure how to fix the PVS-Studio diagnostic. I see two possible causes:

  • PVS-Studio thinks that strint_to_int32 might access the provided out outside the first element. This just does not happen.
  • PVS-Studio thinks that int and int32_t have different sizes. This is not the case with our platform.

@hedger hedger self-requested a review August 13, 2024 15:57
@hedger hedger self-requested a review August 14, 2024 10:25
@portasynthinca3 portasynthinca3 force-pushed the portasynthinca3/3884-int-parsing branch from 6a0e0ef to 945eab6 Compare August 14, 2024 11:29
hedger
hedger previously approved these changes Aug 14, 2024
@hedger hedger added the Core+Services HAL, furi & core system services label Aug 14, 2024
@portasynthinca3
Copy link
Member Author

Thank you, I'll address these comments.

@CookiePLMonster
Copy link
Contributor

Thank you, I'll address these comments.

Looks nice now! Do you think it also makes sense to replace all uses of atoi?

@portasynthinca3
Copy link
Member Author

Looks nice now! Do you think it also makes sense to replace all uses of atoi?

Yep, I think so too. I'll do that tomorrow.

@skotopes skotopes marked this pull request as draft August 16, 2024 10:11
@portasynthinca3 portasynthinca3 marked this pull request as ready for review August 16, 2024 16:55
Copy link
Contributor

@CookiePLMonster CookiePLMonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more nitpicky, but (technically) code-size-reducing comment.

skotopes
skotopes previously approved these changes Sep 5, 2024
@skotopes skotopes merged commit c9791a2 into dev Sep 5, 2024
11 checks passed
@skotopes skotopes deleted the portasynthinca3/3884-int-parsing branch September 5, 2024 17:02
@CookiePLMonster
Copy link
Contributor

@skotopes Does it make sense to strip away atoi, strtol and friends from the firmware/API now that this is available?

@skotopes
Copy link
Member

skotopes commented Sep 5, 2024

@CookiePLMonster yes, but we are not going to do it because external libraries/code may depend on it.

@CookiePLMonster
Copy link
Contributor

Yeah that makes sense, if third parties use it you'll not save any flash space anyway, so might as well keep exposing it...

ofabel pushed a commit to ofabel/flipperzero-firmware that referenced this pull request Sep 26, 2024
* feat: strint_to_uint32 and tests
* fix: permit explicit bases and prefixes
* feat: strint_to_{int32,uint16,int16}
* feat: strint_to_u?int64
* refactor: replace strtol, strtoul, sscanf with strint_to_*
* fix: api symbols
* docs: document parameter `end` of strint_to_uint_32
* style: apply changes requested by hedger
* refactor: fix pvs-studio diagnostic
* style: apply changes requested by CookiePLMonster
* fix: unused var
* fix: pointer type
* refactor: convert atoi to strint_to_*
* fix: strint_to_uint8 doesn't actually exist ._ .
* fix: memory leak
* style: address review comments
* Toolbox: couple small comments in the code and doxygen comment update. SubGhz, Loader: fix strint usage.
* Loader: fix incorrect cast

Co-authored-by: あく <alleteam@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Core+Services HAL, furi & core system services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants