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

time: macOS creates invalid time.Locations using time.LoadLocation #21512

Closed
mconbere opened this issue Aug 18, 2017 · 10 comments
Closed

time: macOS creates invalid time.Locations using time.LoadLocation #21512

mconbere opened this issue Aug 18, 2017 · 10 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mconbere
Copy link

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.8.3

What operating system and processor architecture are you using (go env)?

darwin, amd64

What did you do?

https://play.golang.org/p/zLXHSpuqye

This program on play.golang.org prints:

input: "America/New_York", output: "America/New_York", err: <nil>
input: "America/new_york", output: "UTC", err: cannot find America/new_york in zip file /usr/local/go/lib/time/zoneinfo.zip

On macOS 10.12.4 with a case insensitive HFS+ filesystem it prints:

input: "America/New_York", output: "America/New_York", err: <nil>
input: "America/new_york", output: "America/new_york", err: <nil>

What did you expect to see?

I expected an error to be returned when I ran time.LoadLocation("America/new_york"). These values should be treated as case sensitive. Also potentially valid would be for a valid location to be returned:

loc, _ := time.LoadLocation("America/new_york")
loc.String() == "America/New_York"

What did you see instead?

No error was returned, and a Location was returned with an invalid name. This causes valid zone names on macOS to fail to load on another machine using an identical zoneinfo.

@ianlancetaylor
Copy link
Contributor

The set of supported time zones on a system are always system dependent. They depend entirely on what files are installed on the system.

Closing because I don't see how we can fix it. Please comment if you disagree.

@mconbere
Copy link
Author

mconbere commented Aug 18, 2017

I'm sorry, I didn't explain the bug clearly enough. This bug is related to Go incorrectly parsing the local zoneinfo. Zoneinfo has strict case sensitivity. time.Location.name is set based on the input to LoadLocation. This value is validated by reading the zone file from the case insensitive HFS+ filesystem, and never case-corrected to the on-filesystem case. As a result, any variation of casing will result in LoadLocation succeeding, despite the returned name being an invalid zone.

This happens in line 206 in https://golang.org/src/time/zoneinfo_read.go.

Here is the identical Java program running on the same machine, parsing the same zoneinfo:

import java.util.TimeZone;

public class Location {
  public static void main(String[] args) {
    String[] locInput = {
      "America/New_York",
      "America/new_york"
    };
    for (String input : locInput) {
      TimeZone loc = TimeZone.getTimeZone(input);
      System.out.println("input: " + input + ", output: " + loc.getID());
    }
  }
}

Output:

input: America/New_York, output: America/New_York
input: America/new_york, output: GMT

(TimeZone.getTimeZone returns GMT on error)

See that Java's interpretation of the local zoneinfo is case-sensitive and fails to parse "America/new_york".

@ALTree
Copy link
Member

ALTree commented Aug 18, 2017

Zoneinfo has strict case sensitivity

Paul Eggert, the current editor and maintainer of the tzdata database, has a "Theory and pragmatics of the tz code and data" doc here: https://github.com/eggert/tz/blob/master/Theory.

Regarding case-sensitivity, the doc says:

Here are the general rules used for choosing location names,
in decreasing order of importance:

[...]

[rule 3]
Do not use names that differ only in case. Although the reference
implementation is case-sensitive, some other implementations
are not, and they would mishandle names differing only in case.

The document acknowledges the existence of implementations that are not case-sensitive, so we're not the only outlaws out there.

On the other hand, I quote, "the reference implementation is case-sensitive", so one could argue that it'd be nice to make the Go implementation case-sensitive.

Re-opening, and labelling NeedsDecision.

@ALTree ALTree reopened this Aug 18, 2017
@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 18, 2017
@ALTree ALTree added this to the Go1.10 milestone Aug 18, 2017
@ianlancetaylor
Copy link
Contributor

If we decide to change something, what would we change?

@ALTree
Copy link
Member

ALTree commented Aug 18, 2017

I'd say: if the user asks for America/new_york but on the machine filesystem there's an America/New_York file, we return "not found", even if we are on a case-insensitive file system, where the open(America/new_york) call still works (because the file system ignores case). This would make the time package case-sensitive on macOS. It's already case-sensitive everywhere else (where the file system is case-sensitive).

I'm assuming the macOS filesystem allows you to retrieve the "actual" file name (with the upper case letters). Is this possible?

@rsc
Copy link
Contributor

rsc commented Aug 21, 2017

LoadLocation(name) opens "/usr/share/zoneinfo/"+name. If "america/new_york" exists, then it exists. I don't see how to avoid that. There is no such thing as an official name, and in particular nothing inside the file says "America/New_York". So this is not a bug related to parsing the file data. This data is not in the file.

Time is a very low-level package. It's not at all clear that we should add complexity to try to divine the "official" OS file name just to check for people passing lower case names.

@rsc
Copy link
Contributor

rsc commented Oct 23, 2017

I think we're going to leave this alone.

@rsc rsc closed this as completed Oct 23, 2017
@mconbere
Copy link
Author

I think it's fine to not fix this, but it should be more clearly documented that Go is deviating from the expected convention when you call location.String(), and is unsuitable for use as the canonical name for the zone.

This was surprising to me, and had I seen the documentation earlier I would have implemented an alternative package that gave me access to the actual names, and not the user-input which may or may not be the name.

@ianlancetaylor
Copy link
Contributor

@mconbere Are you suggesting that we add a sentence to the docs for time.(*Location).String along the lines of "Note that this may not be the canonical name of the timezone."?

@rsc
Copy link
Contributor

rsc commented Oct 25, 2017

Go is not deviating from the expected convention: the caller is. time.Location.String just returns the name passed to time.LoadLocation. If the caller passes in a non-standard name to time.LoadLocation, it hardly seems reasonable to fault Go for passing it back.

@golang golang locked and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants