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

[SR-9109] String.init(format:_:), String.init(format:arguments:), String.init(format:locale:_:), String.init(format:locale:arguments), and String.localizedStringWithFormat are dangerous and should be removed. #3601

Open
Lukasa opened this issue Oct 29, 2018 · 5 comments

Comments

@Lukasa
Copy link
Contributor

Lukasa commented Oct 29, 2018

Previous ID SR-9109
Radar rdar://problem/45641203
Original Reporter @Lukasa
Type Bug
Additional Detail from JIRA
Votes 1
Component/s Foundation
Labels Bug
Assignee None
Priority Medium

md5: 97f356ca0132291f075d7a8709f5a166

relates to:

  • SR-10655 Compiler should help to validate string format with variadic arguments (proper CFStringCreateWithFormat support)

Issue Description:

Swift's String contains the following five functions that end up calling into CoreFoundation's CFString formatting APIs:

- String.init(format:_:)
- String.init(format:arguments:)
- String.init(format:locale:_:)
- String.init(format:locale:arguments:)
- String.localizedStringWithFormat(_:_:)

These all have the unappealing property of being able to cause segfaults in pure-Swift code. This is because the underlying CF functions use C variadic arguments (as exposed in the Swift API), and neither CF nor Swift validates that there are the same number of variadic arguments as there are format specifiers in the string.

This set of APIs is simply very dangerous. We should probably do some or all of the following:

1. Document these functions as being only safe to use with static format strings, and as triggering memory-unsafe behaviour if the format specifier and argument count is mismatched.
2. Adjust the function signature to only accept StaticString for the format argument. This makes it harder to misuse (dynamic format strings are almost never going to be right).
3. Replace with the new String interpolation functions.

@belkadan
Copy link

The other thing we've been missing is an equivalent of Clang's -Wformat. That would help too.

@itaiferber
Copy link
Contributor

@belkadan That was my sentiment in the Radar as well — do we have something tracking that?

My thoughts, since there's nothing private about this:

A version of CFStringCreateWithFormat which takes a number of arguments isn’t any better than what we have today — between the format string and the arguments themselves, the source of truth is taken to be the format string. Because of how C is, there’s no way to know how many arguments are passed in, so there’s no way to validate the input against the source of truth

Adding another parameter adds further potential inconsistency with that source of truth:

  • You can mistype the count but have the correct format string and number of arguments

  • You can have the right count and format string, but still pass in the wrong number of arguments

  • You can have the right number of arguments and count, but screw up the format string

  • Rinse and repeat for all combinations, including the count, format string, and arguments being inconsistent

Instead, we have the compiler to verify things in C and Obj-C for CFStringCreateWithFormat and -[NSString initWithFormat:] — both are marked with the same attributes as the printf family of functions so the compiler warns on mismatches and inconsistencies, as it should:

CFStringRef string = CFStringCreateWithFormat(NULL, NULL, CFSTR("%d, %d"), 5); // warning: more '%' conversions than data arguments

If a developer chooses to ignore those warnings, then it’s no surprise if things don’t work.

What we don’t have, though, is a similar facility in Swift:

let str = String(format: "%d, %d", 5)
print(str)

produces no warnings. It would be nice to be able to annotate String.init(format:_🙂 with a similar annotation to keep developers safe in the same way.

@belkadan
Copy link

We have an Apple-internal <rdar://problem/19414352> -Wformat equivalents for Swift, but no JIRA yet. I don't think it's too hard, but it's no Starter Bug either.

@swift-ci
Copy link
Contributor

Comment by Federico Cappelli (JIRA)

@itaiferber @belkadan Any update on this? I think that if we want to maintain the use of CVarArg in swift the compiler should help with the format validation implementing something like the objc `NS_FORMAT_FUNCTION()`

A real-life example is os_log, that is the way-to-go for logging but it's a pain to use, with countless crashes due to wrong format strings.

Something like this compiles with no problems :/

os_log("ups %@, %@, %d", "bug!")

I've created an improvement ticket: https://bugs.swift.org/browse/SR-10655

@itaiferber
Copy link
Contributor

No updates on this yet, but for os_log at least, consider switching to the new osLog interface when available, which does work with string interpolation. I do think that long-term, we'd like to prefer string interpolation over format strings. When everything that format strings can do has made it into the stdlib in one way or another (interpolation is there, but I think we need to offer many more modifiers before it's possible), I'm more than happy to deprecate these.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants