Skip to content

Fix snprintf in minimal-printf library #12632

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
Merged
merged 2 commits into from
Mar 25, 2020
Merged

Fix snprintf in minimal-printf library #12632

merged 2 commits into from
Mar 25, 2020

Conversation

evedon
Copy link
Contributor

@evedon evedon commented Mar 16, 2020

Summary of changes

mbed_minimal_putchar assumed that buffer being NULL meant that it should print to a file. This caused a system crash when calling snprintf with both buffer and stream set to NULL. It is valid to call snprintf with a NULL buffer; nothing should be outputed, but the string length should be measured.

Removed mbed_minimal_formatted_string_character which is no longer required since fputc does the new line conversion. This results in a small Flash saving.

Impact of changes

Fixes #12620

Migration actions required

None

Documentation

None

Pull request type 

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers

@kjbracey-arm

@evedon evedon mentioned this pull request Mar 16, 2020
Closed
@ciarmcom ciarmcom requested review from kjbracey and a team March 16, 2020 18:00
@ciarmcom
Copy link
Member

@evedon, thank you for your changes.
@kjbracey-arm @ARMmbed/mbed-os-test @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 23, 2020

This fixes the issue as confirmed in #12620 (comment)

I'll review shortly, first going to close reopen to restart Travis working

@0xc0170 0xc0170 closed this Mar 23, 2020
@0xc0170 0xc0170 reopened this Mar 23, 2020
0xc0170
0xc0170 previously approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

As soon as commit msg is updated, LGTM. We can start CI

@evedon evedon force-pushed the ed-minimal-snprintf branch from 42703d8 to 3389b0f Compare March 23, 2020 12:54
@evedon
Copy link
Contributor Author

evedon commented Mar 23, 2020

As soon as commit msg is updated, LGTM. We can start CI

Done

@mergify mergify bot dismissed 0xc0170’s stale review March 23, 2020 12:55

Pull request has been modified.

0xc0170
0xc0170 previously approved these changes Mar 23, 2020
View reviewed changes
@0xc0170 0xc0170 self-requested a review March 23, 2020 13:03
evedon added 2 commits March 23, 2020 13:46
@evedon
Fix snprintf in minimal-printf library.
94acb5d 
mbed_minimal_putchar assumed that buffer being NULL meant that it
should print to a file. This caused a system crash when calling
snprintf with both buffer and stream set to NULL.
It is valid to call snprintf with a NULL buffer; nothing should
be outputted, but the string length should be measured.
@evedon
Removed mbed_minimal_formatted_string_character
a3c3656 
mbed_minimal_formatted_string_character is no longer
required since fputc does the new line conversion.
This results in a small Flash saving.
@evedon evedon force-pushed the ed-minimal-snprintf branch from 3389b0f to a3c3656 Compare March 23, 2020 13:52
@mergify mergify bot dismissed 0xc0170’s stale review March 23, 2020 13:52

Pull request has been modified.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 23, 2020

CI started

@mergify mergify bot added needs: CI and removed needs: work labels Mar 23, 2020
@mbed-ci
Copy link

mbed-ci commented Mar 23, 2020

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mergify mergify bot added needs: work and removed needs: CI labels Mar 23, 2020
@mergify mergify bot removed the needs: CI label Mar 23, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

Test restarted. This should get in today

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

Disco target fails few test cases for devicekey, I restarted it to make sure it's there. Please review failures

@evedon
Copy link
Contributor Author

evedon commented Mar 24, 2020

test_direct_access_to_devicekey_tdb_flashiap_default fails. @SeppoTakalo can you help?

@SeppoTakalo
Copy link
Contributor

The test fail in:

    err = tdb->init();
    TEST_ASSERT_EQUAL_ERROR_CODE(0, err);

Where TDBStore::init() returned READ_FAILED

This sounds like a broken hardware.. Reading a flash should not easily fail.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020 •
edited
Loading

Also PR #12683 , similar failure. Will talk to @ARMmbed/mbed-os-test to check the board

@hugueskamba
Copy link
Collaborator

@0xc0170
I ran the failing test (features-storage-tests-kvstore-direct_access_devicekey_test) on Windows (mbed test -m DISCO_L475VG_IOT01A -t arm -n features-storage-tests-kvstore-direct_access_devicekey_test) with this branch and it repeatedly passes fine. Has the ST firmware on the board been updated?

@0xc0170 0xc0170 mentioned this pull request Mar 25, 2020
Closed
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 25, 2020

Tests restarted

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 25, 2020 •
edited
Loading

Tests passed 🎉

@0xc0170 0xc0170 merged commit ddd3b01 into ARMmbed:master Mar 25, 2020
@mergify mergify bot added release version missing When PR does not contain release version, bot should label it and we fix it afterwards and removed ready for merge labels Mar 25, 2020
@mergify
Copy link

mergify bot commented Mar 25, 2020

This PR does not contain release version label after merging.

@0xc0170 0xc0170 removed the release version missing When PR does not contain release version, bot should label it and we fix it afterwards label Mar 25, 2020
@evedon evedon deleted the ed-minimal-snprintf branch March 26, 2020 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@kivaisan kivaisan kivaisan approved these changes

@kjbracey kjbracey Awaiting requested review from kjbracey

Assignees
No one assigned
Labels
release-version: 6.0.0-beta-1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using mbed_trace_prefix_function_set causes crash
7 participants
@evedon @ciarmcom @0xc0170 @mbed-ci @SeppoTakalo @hugueskamba @kivaisan