Skip to content

tests/pkg_fatfs: fix sprintf format overflow warning#12503

Merged
gschorcht merged 1 commit intoRIOT-OS:masterfrom
kaspar030:fix_tests/pkg_fatfs_format_overflow
Oct 19, 2019
Merged

tests/pkg_fatfs: fix sprintf format overflow warning#12503
gschorcht merged 1 commit intoRIOT-OS:masterfrom
kaspar030:fix_tests/pkg_fatfs_format_overflow

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 commented Oct 18, 2019 •
edited
Loading

Contribution description

Some platforms issue this:

    tests/pkg_fatfs/main.c: In function '_mkfs':
    tests/pkg_fatfs/main.c:355:26: error: '%d' directive writing between 1 and 11 bytes into a region of size 8 [-Werror=format-overflo
    w=]
      355 |     sprintf(volume_str, "%d:/", vol_idx);
          |                          ^~
    tests/pkg_fatfs/main.c:355:25: note: directive argument in the range [-2147483648, 0]
      355 |     sprintf(volume_str, "%d:/", vol_idx);
          |                         ^~~~~~
    tests/pkg_fatfs/main.c:355:5: note: 'sprintf' output between 4 and 14 bytes into a destination of size 8
      355 |     sprintf(volume_str, "%d:/", vol_idx);
          |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    cc1: all warnings being treated as errors

The cause is vol_idx being converted by atoi(), so it might be any negative int, which the later code doesn't check for.
This commit increases the stack array so it cannot overflow. It probably increases stack usage by 6-8 bytes, but this is a test shell command, so I think that's acceptable...

Testing procedure

Caught this in #12457 (with TI's toolchain). Somehow this doesn't trigger in master.
Maybe a good look at the code is sufficient?

Issues/PRs references

#12457 fails compiling the test due to this.

@kaspar030
tests/pkg_fatfs: fix sprintf format overflow warning
ea6c4a7 
Some platforms issue this:

    tests/pkg_fatfs/main.c: In function '_mkfs':
    tests/pkg_fatfs/main.c:355:26: error: '%d' directive writing between 1 and 11 bytes into a region of size 8 [-Werror=format-overflo
    w=]
      355 |     sprintf(volume_str, "%d:/", vol_idx);
          |                          ^~
    tests/pkg_fatfs/main.c:355:25: note: directive argument in the range [-2147483648, 0]
      355 |     sprintf(volume_str, "%d:/", vol_idx);
          |                         ^~~~~~
    tests/pkg_fatfs/main.c:355:5: note: 'sprintf' output between 4 and 14 bytes into a destination of size 8
      355 |     sprintf(volume_str, "%d:/", vol_idx);
          |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    cc1: all warnings being treated as errors

The cause is vol_idx being converted by atoi(), so it might be negative.
This commit increases the stack array so it cannot overflow.
@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 18, 2019
@kaspar030 kaspar030 mentioned this pull request Oct 18, 2019
Merged
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Oct 18, 2019

@kb2ma This hits in a rare corner case (user gives too large negative number as vol_idx parameter) and might cause stack corruption. Not sure this warrants the effort to backport.

@kb2ma
Copy link
Copy Markdown
Member

kb2ma commented Oct 18, 2019

Thanks for the mention. I'll look into it.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Oct 18, 2019

Thanks for the mention. I'll look into it.

We can probably get away with a known issue entry.

gschorcht
gschorcht reviewed Oct 19, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@gschorcht gschorcht left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Make sense. A character buffer with 8 bytes is too small. For 32 bit integers, 14 characters are required including the sign and the terminating \0.

@gschorcht gschorcht added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Oct 19, 2019
@gschorcht
Copy link
Copy Markdown
Contributor

gschorcht commented Oct 19, 2019

Tested with new and old MSP430 toolchain.

@gschorcht gschorcht merged commit 855ef72 into RIOT-OS:master Oct 19, 2019
@kaspar030 kaspar030 deleted the fix_tests/pkg_fatfs_format_overflow branch October 19, 2019 08:52
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Oct 19, 2019

Thanks!

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Oct 19, 2019

We can probably get away with a known issue entry.

Could it be #8408 ? If yes, we can close it.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Oct 19, 2019

Could it be #8408 ? If yes, we can close it.

I think that's unrelated. The format overflow actually happens on all platforms.

@kb2ma
Copy link
Copy Markdown
Member

kb2ma commented Oct 19, 2019 •
edited
Loading

So let me make sure I understand. The issue is a compiler error on a test, and we're just now seeing this because we're using a modern TI toolchain? If so, I don't see running tests with this toolchain on a released version as something worth a backport.

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Oct 19, 2019

No, the issue is a not properly checked argument (vol_idx) in a test shell command, triggering a valid sprintf "format overflow" warning.
The argument is supposed to be (0-(MTD_NUM-1)). It is converted using atoi(). It is possible to pass negative numbers, which is not checked for. That can lead to an sprintf overflowing the target buffer.

This PR mitigates the buffer overflow by increasing the buffer. The code will probably error later on, anyways. I didn't check that.

I saw this warning trigger with the TI toolchain. Maybe different default warning flags?
I've also seen it when compiling RIOT differently (experimenting with build systems) on arm and native.

@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gschorcht gschorcht gschorcht approved these changes

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

Assignees

No one assigned

Labels

Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2020.01

Development

Successfully merging this pull request may close these issues.

5 participants

@kaspar030 @kb2ma @gschorcht @aabadie @fjmolinas