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

Fixed parsing hex numbers in chip config files #1169

Merged
merged 3 commits into from
Aug 1, 2021
Merged

Fixed parsing hex numbers in chip config files #1169

merged 3 commits into from
Aug 1, 2021

Conversation

gszy
Copy link
Collaborator

@gszy gszy commented Jul 29, 2021

process_chipfile() used to improperly parse hex numbers in chip config files (*.chip), because it used atoi(), which read all such numbers as 0. This resulted, among other issues, in chip_id being set to 0 for all read chips. Such chip id could not match any actual MCU.

Replace the atoi() calls with sscanf(…, "%i", …), where %i should match integers in base 10, 8 and 16, depending on the number prefix.


I can rebase this PR once (if) #1166 is merged.

@Nightwalker-87
Copy link
Member

Nightwalker-87 commented Jul 29, 2021

@gszy: Please check for build failures, e.g.:

/home/runner/work/stlink/stlink/src/stlink-lib/chipid.c:1001:27: error: format ‘%i’ expects argument of type ‘int *’, but argument 3 has type ‘enum stlink_flash_type *’ [-Werror=format=]

You may also collect several issues in one PR for your convenience in the future - it shall be fine as well.

@Nightwalker-87 Nightwalker-87 self-requested a review July 29, 2021 23:13
process_chipfile() used to improperly parse hex numbers in chip config
files (*.chip), because it used atoi(), which read all such numbers
as 0. This resulted, among other issues, in chip_id being set to 0 for
all read chips. Such chip id could not match any actual MCU.

Replace the atoi() calls with sscanf(…, "%i", …), where %i should match
integers in base 10, 8 and 16, depending on the number prefix.
@gszy
Copy link
Collaborator Author

gszy commented Jul 30, 2021

Sorry for the failures—now it should build cleanly, though I’m not sure if that’s a solution you approve:

// may set invalid flash types (not defined in enum stlink_flash_type)
if (sscanf(value, "%i", (int *) &ts->flash_type) < 1)

@Ant-ON
Copy link
Collaborator

Ant-ON commented Jul 30, 2021

@gszy This is a good solution to the problem!
Also you can add a check that this type of flash is supported (included in the range). To do this, you can add something like STLINK_FLASH_TYPE_MAX and check that the type is between 0 and maximum. This will help avoid problems with misconfigurations in the future.

Add a STLINK_FLASH_TYPE_MAX constant that can be used to check if a
flash type (an integer) is in the range of valid enum stlink_flash_type
values.
src/stlink-lib/chipid.c Outdated Show resolved Hide resolved
@gszy
Copy link
Collaborator Author

gszy commented Jul 30, 2021

You may also collect several issues in one PR for your convenience in the future - it shall be fine as well.

I’m trying to make my PRs small and easy to review 🙂

Also you can add a check that this type of flash is supported (included in the range). To do this, you can add something like STLINK_FLASH_TYPE_MAX and check that the type is between 0 and maximum.

I have added this check in separate commits (feel free to cherry pick what you find useful). There should be some range checks for other parameters as well, but I hope for another PR when I (or someone else) will find more time.

@Ant-ON
Copy link
Collaborator

Ant-ON commented Jul 30, 2021

@gszy Thank you!
Typos in other parameters should not result to segmentation failures. This is a critical parameter. Good job!

@Nightwalker-87 Nightwalker-87 changed the title Fix parsing hex numbers in chip config files Fixed parsing hex numbers in chip config files Jul 31, 2021
@Nightwalker-87 Nightwalker-87 merged commit 9605d21 into stlink-org:develop Aug 1, 2021
@stlink-org stlink-org locked as resolved and limited conversation to collaborators Aug 1, 2021
@gszy gszy deleted the fix-parsing-hexs branch August 1, 2021 15:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants