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

STM32L-Discovery (STM32L152R8T6) can't write firmware to chip with st-flash on Windows 10 #1214

Closed
5 tasks done
hydroconstructor opened this issue Jan 9, 2022 · 13 comments · Fixed by #1210
Closed
5 tasks done

Comments

@hydroconstructor
Copy link
Collaborator

hydroconstructor commented Jan 9, 2022

  • Programmer/board type: STM32L-Discovery, STLINK /V2-onboard
  • Operating system and version: Windows 10 x64
  • stlink tools version: 1.7.0, current develop branch
  • stlink commandline tool name: st-flash
  • Target chip and board: STM32L152R8T6, board STM32L-Discovery

Commandline output:

mmap() size_t overflow for file mb.bin

Expected/description:

Debugging revealed that problem is in file common.c, function map_file, in code below:

  if (sizeof(st.st_size) != sizeof(size_t)) {
    // on 32 bit systems, check if there is an overflow
    if (st.st_size > (off_t)SSIZE_MAX) {
      fprintf(stderr, "mmap() size_t overflow for file %s\n", path);
      goto on_error;
    }
  }

Type of st.st_size is off_t, which means long int. Value is size of firmware file and for me is 602 bytes.
SSIZE_MAX defines somewhere in libc headers. It means maximum size of signed integer. But in my system it compiled in long long int. Not 4 bytes long int as off_t, but 8 bytes long long int! So there is overflow in casting types without warnings during compile. As a result, instead of:
if (602 > 2^32-1) {
I got:
if (602 > -1) {
And then function returned with error message.

Proposed solution, which works on my computer:
In header section of common.c define macro OFF_T_MAX such a way:

#ifndef OFF_T_MAX
#define OFF_T_MAX 1073741824 // long int max value
#endif

In map_file function replace SSIZE_MAX by newly defined OFF_T_MAX.

Also please note that SSIZE_MAX defined in unistd.h in such a way:

#define ssize_t int
#ifndef SSIZE_MAX
#define SSIZE_MAX ((sizeof(ssize_t) == 4) ? 1073741824 : ‭4611686018427387904‬)
#endif

There is an error in numbers. SSIZE_MAX is max. value of signed int and must be equal 0x400... . But here it equals 0x7FFF ... in both cases, which is true for UNsigned int. For signed int both values are -1. But in my system SSIZE_MAX defined in libc, so this macro is not used.

@Nightwalker-87
Copy link
Member

To me it makes absolutely no sense to define ssize_t as type int because as per design it should never turn to negative values. Thus please correct the type to uint first before further testing. Afterwards we should have another look the specific problem. It appears better to proceed in this way to rule out unintended side effects. Please also take note of #909 which is related to this topic. As it appears several variables appear not to make use of correct types.

@hydroconstructor
Copy link
Collaborator Author

hydroconstructor commented Jan 9, 2022

Never turn to negative? Sorry, but can't agree. Many functions return -1 in case of errors. For example, send_recv from #909 mentioned by you. Refactor them all is too difficult - at least for me, these are my first steps in programming.
So if you do not mind I prepare next commit with changes in common.c described above. File unistd.h with #define ssize_t int should be refactored with all other files.

@Ant-ON
Copy link
Collaborator

Ant-ON commented Jan 9, 2022

@hydroconstructor I think this check

  if (sizeof(st.st_size) != sizeof(size_t)) {
    // on 32 bit systems, check if there is an overflow
    if (st.st_size > (off_t)SSIZE_MAX) {
      fprintf(stderr, "mmap() size_t overflow for file %s\n", path);
      goto on_error;
    }
  }

may be replaced by

  if (st.st_size > (1<<20) /*1GB*/) {
    // limit file size to 1GB
    fprintf(stderr, "mmap() file %s too big\n", path);
    goto on_error;
  }

The sizes of the MCU firmware will not go beyond this size for a long time, while we will eliminate the wrong choice of files.

@hydroconstructor
Copy link
Collaborator Author

Good idea. Doing this just now...

@Nightwalker-87
Copy link
Member

@hydroconstructor One needs to distinguish between functions and variables. I was talking about types for variables.
A function may return an error, but not a variable itself which is figuratively only a "number".

@hydroconstructor
Copy link
Collaborator Author

@Nightwalker-87, functions have types. Type of send_recv from #909 is ssize_t, and it returns -1 in case of error. There are other functions with type ssize_t, returning -1 in case of error. If I make global change from #define ssize_t int to #define ssize_t uint for all project code, I should check each such function and change it's return value if necessary, right?

@Ant-ON
Copy link
Collaborator

Ant-ON commented Jan 9, 2022

@Nightwalker-87 Here the problem is in the specific library libc, which has a non-standard type st_size. An error occurs due to the different dimension and the С types conversion.

@Nightwalker-87
Copy link
Member

Nightwalker-87 commented Jan 9, 2022

@Ant-ON What would you propose as the most reasonable approach to deal with this?

@Nightwalker-87
Copy link
Member

@hydroconstructor In general yes, but we need to be careful regarding possible side effects. As many contributors have worked on or modified the code there is no clear concept regarding this and nobody really seems to completely overlook the current state. This is also one reason why #909 was opened. It is clear that this topic wants some work being done, but I'm not sure how far we should go here within this ticket. Therefore I asked @Ant-ON for his opinion on this.

@Nightwalker-87 Nightwalker-87 changed the title STM32L152R8T6: STM32L-Discovery, on Windows 10 can't write firmware to chip by st-flash through ST-Link v. 2 STM32L-Discovery (STM32L152R8T6) can't write firmware to chip with st-flash on Windows 10 Jan 9, 2022
hydroconstructor added a commit to hydroconstructor/stlink that referenced this issue Jan 9, 2022
Error in file size comparizon.
Due to type casting, instead of compare file size with max. singed int value, it compares with -1. Then function returns with error message.
@Ant-ON
Copy link
Collaborator

Ant-ON commented Jan 9, 2022

@Nightwalker-87 This problem is similar to #909 but not related to #909. In this case, the problem is in the 3th party library.
I have suggested the most simple and reasonable solution in the post above.

@Nightwalker-87
Copy link
Member

Ok, fine - I noticed it has already been pushed by @hydroconstructor as well. 👍

@gszy
Copy link
Collaborator

gszy commented Jan 15, 2022

st_size from struct stat is of type off_t, which, per system_data_types(7):

According to POSIX, this shall be a signed integer type.

From this point of view, max st_size value may be less than 1 << 20.


Back to the original issue: we have to check if our file size, st.st_size, is larger than max size_t value, since we want to pass it as mmap(2)’s size_t length argument:

if (st.st_size > SIZE_MAX) {/*…*/} // C99 stdint.h

This way we would probably get a compiler warning, because off_t and type of SIZE_MAX may be of different signedness. Let’s cast the latter to intmax_t:

if (st.st_size > (intmax_t) SIZE_MAX) {/*…*/} // C99 stdint.h

This version will work as long as INTMAX_MAX >= SIZE_MAX. I cannot yet prove that is always the case, but please note that, if INTMAX_MAX < SIZE_MAX, then SSIZE_MAX < SIZE_MAX as well. (st.st_size will get automatically promoted.)


I’m sorry for coming here so late (now that there is a ready to ship PR) and I’m sorry if I have missed something obvious and we cannot actually use the above solution. HTH.

@Nightwalker-87
Copy link
Member

@gszy That's ok, don't worry. Just proceed as intended.

@stlink-org stlink-org locked as resolved and limited conversation to collaborators Jan 23, 2022
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 a pull request may close this issue.

4 participants