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.

Sign up for GitHub

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

Jump to bottom

AP_ROMFS: remove requirement that returned buffer is null-terminated #26184

Closed
wants to merge 4 commits into from
Closed

AP_ROMFS: remove requirement that returned buffer is null-terminated #26184

wants to merge 4 commits into from

Conversation

tpwrules
Copy link
Contributor

@tpwrules tpwrules commented Feb 10, 2024 •
edited

Saves (a very small amount of) code size and improves clarity. See commits for details.

Tested that a CubeOrange can flash its own bootloader. I can't test the bootloader changes as I don't have a compatible board.

@tpwrules
AP_HAL_ChibiOS: remove redundant fw size check
9cc889e 
embed.py already ensures it's a multiple of 32 bytes. If it wasn't, then
the buffer would be overflowed and random garbage would be flashed at
the end.
@tpwrules
AP_Param: clarify use of parameter text buffer
075afb2 
Refer to it 'buffer' instead of 'string' to clarify that the length is
respected and that it does not need to be null terminated.
@tpwrules
AP_Bootloader: don't assume substitute_vars input is null-terminated
42e5c9c
@tpwrules
AP_ROMFS: remove requirement that returned buffer is null-terminated
e754938 
Also take the opportunity to clarify the function descriptions.
@tridge
Copy link
Contributor

tridge commented Feb 19, 2024

@tpwrules I've been thinking about this and I think it would be better to guarantee nul termination for both compressed and uncompressed ROMFS files, but with the returned length reflecting the true length (so not including the nul if it has been added to ensure termination)
otherwise we end up with a bear trap for future developers where they think they are getting a text file that they can run string operations on and it works most of the time but occasionally dies horribly
So I'm basically erring on the side of robustness and ease of use for the API
I've added dev call topic as others may disagree and its worth discussing

@tpwrules
Copy link
Contributor Author

It currently works how you describe, and I agree that "occasionally dies horribly" is a really bad property for this type of software.

But it confuses me how future developers would end up with the impression that "text file" == "string operations". AP_Filesystem::load_file() does not terminate the returned data and I'm not aware of any other file API on any other system that does.

The only case I found which relies on the null termination behavior, namely BL_Network::substitute_vars, is really just a bug: it takes in the length of the buffer, but sometimes can access past it, which is contrary to good programming practice. I'd say that as a rule string operations should not be used in the code without an explicit length.

@davidbuzz
Copy link
Collaborator

... if we aren't going to guarantee it, then maybe we need a suite of tests that cover all the corner cases to ensure it.?

@tpwrules
Copy link
Contributor Author

tpwrules commented Feb 20, 2024 •
edited

I'm going to propagate this property to AP_Filesystem::load_file()instead of removing it from ROMFS.

I maintain that it's not correct to rely on if given a length and plan to file issues to fix.

@tpwrules tpwrules closed this Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tpwrules @tridge @davidbuzz @CraigElder