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-14941] Introduce compiler warning for leading-zero octal notation #57283

Open
swift-ci opened this issue Jul 19, 2021 · 12 comments
Open

[SR-14941] Introduce compiler warning for leading-zero octal notation #57283

swift-ci opened this issue Jul 19, 2021 · 12 comments

Comments

@swift-ci
Copy link
Collaborator

@swift-ci swift-ci commented Jul 19, 2021

Previous ID SR-14941
Radar rdar://problem/22123688
Original Reporter eaigner (JIRA User)
Type Improvement
Additional Detail from JIRA
Votes 0
Component/s
Labels Improvement, StarterBug
Assignee None
Priority Medium

md5: 639a9bd96ac9c3918c5433098fa9ad29

Issue Description:

I had a bug in my code for quite some time that went by unnoticed, because I didn't realize Swift handled octal numbers differently. Specifically I was making the following call

mkfifo(path, 0644)

This should obviously read `0o644`, but went by unnoticed, since it just resulted in an elevated user permission.

It would be great if the compiler could warn in such instances where numbers are typed with leading zeroes, maybe something of the form "Did you mean to write an octal number?"

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Jul 19, 2021

I agree, this is a good idea.

There is one edge case though, if someone is using a leading zero for consistency reasons, say in a literal like [0055, 0550, 5500, 0505, 5005, 5050, 5500], then this warning will give a bunch of false positives, which would be unfortunate...

@swift-ci create

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Jul 19, 2021

Comment by Erik Aigner (JIRA)

I would say a potential security issue is more important then using 0 as padding. If someone wanted to line up a number matrix (or I don't know what your edge case would be), he could as well substitute a space for the leading zero.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Jul 19, 2021

Comment by Erik Aigner (JIRA)

In any case, if leading zeroes are wanted, there is always the option to disable certain compiler warnings.

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Jul 19, 2021

I would say a potential security issue is more important then using 0 as padding.

I agree, that's why I said this is a good idea overall. It's just that we need to provide a way to suppress the warning if it is superfluous.

there is always the option to disable certain compiler warnings

That's not the case in general. Certain warnings have ad-hoc ways of being suppressed, but most warnings cannot be disabled on an individual basis.

For this particular one, we'd probably need to come up with a way to suppress it locally if we were to add it.

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Jul 26, 2021

I think the right place to do this would in the implementation of CSApply.cpp's {{handleIntegerLiteralExpr}} function.

Under the following circumstances:

  • The literal has a leading zero not following by an 'o'.

  • All digits are in the range 0-7.

  • The expression is an argument to a function which was imported from C where the corresponding argument type is 'mode_t'.

In this situation, emit a warning with a fix-it to add an 'o' after the 0.

@idrougge
Copy link

@idrougge idrougge commented Jul 30, 2021

I think this is quite undesirable. Leading zeros are very useful for auto-generated or tabulated literals — the fact that they have another meaning in another language (whose example Swift has chosen not to follow) is irrelevant.

Generating a warning for no specific reason makes leading zeros basically useless.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Jul 30, 2021

Comment by Erik Aigner (JIRA)

What exactly prevents you from putting a leading space instead of a leading zero? Where is it written that tabulated data has to have leading zeroes?

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Jul 30, 2021

Comment by Erik Aigner (JIRA)

Also, since swift chose to support _ for number readability, you might as well allow _ as a prefix instead of abusing 0.

@typesanitizer
Copy link

@typesanitizer typesanitizer commented Jul 31, 2021

@idrougge, that is why I proposed a very narrow warning, not for arbitrary situations involving leading zeroes. Swift needs to talk to C APIs; in fact, Swift has a special ClangImporter sub-system to make importing APIs from C-based languages easy.

Dismissing someone else's concern as "irrelevant" is not particularly helpful. In this bug report, there is a very concrete use case that has been given, which can lead to a security issue. I don't think anyone is arguing that all leading zeroes should lead to warnings. Adding a targeted warning should be doable without issuing additional warnings for the vast majority of Swift code using leading zeroes.

@xwu
Copy link
Collaborator

@xwu xwu commented Jul 31, 2021

In the same vein, though, dismissing the legitimate use of leading zeros as “abuse,” when it is not only conventional mathematic notation but so commonly a source of error in C-family programming languages that it is the reason for Swift to deviate from C octal notation, is not appropriate.

I find the maxim that everything permitted in a programming language will be used by somebody to hold true generally, and given that leading zeros have been explicitly contemplated since at least the very first public release of Swift to mean what it does today, we should be very careful about how to proceed here. If somehow a warning can be very narrowly tailored (say, only when interfacing with C APIs that take arguments of type mode_t, just as Varun argues, or creating some sort of annotation for APIs that typically take octal arguments), then that seems plenty wise.

But the use of leading zeros is most certainly not an “abuse” (nor “edge case,” given that it’s the crux of the rationale for Swift’s decision to deviate from prior art), and warning about all such uses (which is, by my read, what Erik here is arguing for) in my view is a nonstarter.

Overall, I think there’s sufficient design finesse and subject matter expertise required here for an appropriate solution that I wouldn’t characterize it as a starter bug.

@swift-ci
Copy link
Collaborator Author

@swift-ci swift-ci commented Jul 31, 2021

Comment by Erik Aigner (JIRA)

I am not arguing for anything except a warning for octal permission flags for C APIs. I couldn't care less about leading zeroes, as nonsensical they might be.

@xwu
Copy link
Collaborator

@xwu xwu commented Jul 31, 2021

Sure, I think a warning for that scenario is pretty reasonable. Again, though, let’s not disparage other people’s coding preferences as “nonsensical.” It doesn’t advance your point and comes across as antagonistic.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 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