This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Fixes an off by one error.
ClosedPublic

Authored by Mordante on Jul 16 2023, 2:37 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG7583c73bc4fa: [libc++][format] Fixes an off by one error.
Summary

The post-condition on the functions is that the buffer is not full.
This post-conditon is used as pre-condition of the push_back function.
When a copy, fill, of transform function exactly fit in the buffer this
post-condition was validated.

Diff Detail

Event Timeline

Mordante created this revision.Jul 16 2023, 2:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2023, 2:37 AM
Mordante published this revision for review.Jul 16 2023, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2023, 5:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Jul 17 2023, 7:48 AM
ldionne added a subscriber: ldionne.

Did you find that with fuzzing?

This revision is now accepted and ready to land.Jul 17 2023, 7:48 AM
This revision was automatically updated to reflect the committed changes.
dim added a subscriber: dim.Jul 17 2023, 9:51 AM

Did you find that with fuzzing?

This was reported in the security bug tracker at https://bugs.chromium.org/p/llvm/issues/detail?id=48 . That issue was not accessible at first, but I have removed the security restriction now.

avogelsgesang added a subscriber: avogelsgesang.EditedJul 18 2023, 5:28 AM

Did you find that with fuzzing?

We (a small team inside Salesforce) found this in one of our production builds by accident / sheer luck. We are using -fexperimental-library already, because we recompile the world anyway (and don't need ABI stability) and are happy to rewrite our code in case breaking API changes are applied to libc++ or the C++ standard.

I agree that fuzzing might be a good idea for std::format to find this type of issues in a more structured way

Did you find that with fuzzing?

We (a small team inside Salesforce) found this in one of our production builds by accident / sheer luck. We are using -fexperimental-library already, because we recompile the world anyway (and don't need ABI stability) and are happy to rewrite our code in case breaking API changes are applied to libc++ or the C++ standard.

I agree that fuzzing might be a good idea for std::format to find this type of issues in a more structured way

Thanks for the context!

Did you find that with fuzzing?

This was reported in the security bug tracker at https://bugs.chromium.org/p/llvm/issues/detail?id=48 . That issue was not accessible at first, but I have removed the security restriction now.

I indeed mention this privately to @ldionne since at that time the issue was still restricted.

Did you find that with fuzzing?

We (a small team inside Salesforce) found this in one of our production builds by accident / sheer luck. We are using -fexperimental-library already, because we recompile the world anyway (and don't need ABI stability) and are happy to rewrite our code in case breaking API changes are applied to libc++ or the C++ standard.

I agree that fuzzing might be a good idea for std::format to find this type of issues in a more structured way

+1 This is already high on my priority list. D154140 enables fuzzing in our CI so when that lands I will look into fuzzing.