Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v5]
On Tue, 19 Oct 2021 04:08:12 GMT, Wu Yan wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > Wu Yan has updated the pull request incrementally with one additional commit > since the last revision: > > fix code style Looks good. Thanks for the fix. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 13:39:46 GMT, Naoto Sato wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > The change looks reasonable. Please test your fix with macOS as well. Hi, @naotoj, I pushed the new commit, do you need to review again? - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v5]
On Tue, 19 Oct 2021 04:08:12 GMT, Wu Yan wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > Wu Yan has updated the pull request incrementally with one additional commit > since the last revision: > > fix code style looks good - Marked as reviewed by mli (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v4]
On Fri, 15 Oct 2021 07:03:18 GMT, Hamlin Li wrote:
>> Wu Yan has updated the pull request with a new target base due to a merge or
>> a rebase. The incremental webrev excludes the unrelated changes brought in
>> by the merge/rebase. The pull request contains four additional commits since
>> the last revision:
>>
>> - Merge branch 'master' into timezone
>> - change functions to be static
>> - replace realpath
>> - 8273111: Default timezone should return zone ID if /etc/localtime is
>> valid but not canonicalization on linux
>
> src/java.base/unix/native/libjava/TimeZone_md.c line 82:
>
>> 80:
>> 81: /*
>> 82: * remove repeated path separators ('/') in the giving 'path'.
>
> given?
Thanks, fix it.
> src/java.base/unix/native/libjava/TimeZone_md.c line 89:
>
>> 87: char *left = path;
>> 88: char *right = path;
>> 89: char *end = path + strlen(path);
>
> "char* end"? better to align with existing code style
OK, fixed it.
> src/java.base/unix/native/libjava/TimeZone_md.c line 95:
>
>> 93: for (; right < end; right++) {
>> 94: if (*right == '/' && *(right + 1) == '/') break;
>> 95: }
>
> Is this for loop necessary? Seems it's ok to merge with the nested loop below.
Thanks for your suggestion, this for loop is indeed unnecessary. Removed it.
-
PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v5]
> Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Wu Yan has updated the pull request incrementally with one additional commit since the last revision: fix code style - Changes: - all: https://git.openjdk.java.net/jdk/pull/5327/files - new: https://git.openjdk.java.net/jdk/pull/5327/files/93cff3d1..ba9cc656 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5327=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5327=03-04 Stats: 9 lines in 1 file changed: 0 ins; 6 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v4]
On Thu, 14 Oct 2021 02:08:45 GMT, Wu Yan wrote:
>> Hi,
>> Please help me review the change to enhance getting time zone ID from
>> /etc/localtime on linux.
>>
>> We use `realpath` instead of `readlink` to obtain the link name of
>> /etc/localtime, because `readlink` can only read the value of a symbolic of
>> link, not the canonicalized absolute pathname.
>>
>> For example, the value of /etc/localtime is
>> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by
>> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of
>> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which
>> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you
>> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“.
>>
>> Thanks,
>> wuyan
>
> Wu Yan has updated the pull request with a new target base due to a merge or
> a rebase. The incremental webrev excludes the unrelated changes brought in by
> the merge/rebase. The pull request contains four additional commits since the
> last revision:
>
> - Merge branch 'master' into timezone
> - change functions to be static
> - replace realpath
> - 8273111: Default timezone should return zone ID if /etc/localtime is valid
> but not canonicalization on linux
src/java.base/unix/native/libjava/TimeZone_md.c line 82:
> 80:
> 81: /*
> 82: * remove repeated path separators ('/') in the giving 'path'.
given?
src/java.base/unix/native/libjava/TimeZone_md.c line 89:
> 87: char *left = path;
> 88: char *right = path;
> 89: char *end = path + strlen(path);
"char* end"? better to align with existing code style
src/java.base/unix/native/libjava/TimeZone_md.c line 95:
> 93: for (; right < end; right++) {
> 94: if (*right == '/' && *(right + 1) == '/') break;
> 95: }
Is this for loop necessary? Seems it's ok to merge with the nested loop below.
-
PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v4]
> Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Wu Yan has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'master' into timezone - change functions to be static - replace realpath - 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux - Changes: - all: https://git.openjdk.java.net/jdk/pull/5327/files - new: https://git.openjdk.java.net/jdk/pull/5327/files/38177cd0..93cff3d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5327=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5327=02-03 Stats: 1449026 lines in 13982 files changed: 735454 ins; 659127 del; 54445 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v3]
On Wed, 13 Oct 2021 09:15:25 GMT, Wu Yan wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > Wu Yan has updated the pull request incrementally with one additional commit > since the last revision: > > change functions to be static Looks good. Thanks for the fix. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v3]
> Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Wu Yan has updated the pull request incrementally with one additional commit since the last revision: change functions to be static - Changes: - all: https://git.openjdk.java.net/jdk/pull/5327/files - new: https://git.openjdk.java.net/jdk/pull/5327/files/19cc634d..38177cd0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5327=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5327=01-02 Stats: 6 lines in 2 files changed: 0 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v2]
On Mon, 11 Oct 2021 18:18:02 GMT, Naoto Sato wrote: >> Wu Yan has updated the pull request incrementally with one additional commit >> since the last revision: >> >> replace realpath > > src/java.base/unix/native/libjava/path_util.h line 31: > >> 29: int collapsible(char *names); >> 30: void splitNames(char *names, char **ix); >> 31: void joinNames(char *names, int nc, char **ix); > > Are these functions, `collapsible`, `splitNames` and `joinNames` have to be > non-static? You are right, thanks for your suggestions, I'll change these functions to static in next commit. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v2]
On Mon, 11 Oct 2021 18:16:28 GMT, Naoto Sato wrote: >> Wu Yan has updated the pull request incrementally with one additional commit >> since the last revision: >> >> replace realpath > > src/java.base/unix/native/libjava/TimeZone_md.c line 113: > >> 111: } >> 112: } >> 113: > > There are a few `*(right + 1)` references in the loops. Is there any > possibility that it would run over the buffer? It wouldn't run over the buffer. Here `end` points to `'\0'`. At line 94 and line 99, `right` is less than `end`, so `right + 1` is at most `end`. At line 103, if `right` equals `end`, `*right != '\0'` will be false and `!(*right == '/' && *(right + 1) == '/')` will not executed. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v2]
On Thu, 9 Sep 2021 08:25:44 GMT, Wu Yan wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > Wu Yan has updated the pull request incrementally with one additional commit > since the last revision: > > replace realpath src/java.base/unix/native/libjava/TimeZone_md.c line 113: > 111: } > 112: } > 113: There are a few `*(right + 1)` references in the loops. Is there any possibility that it would run over the buffer? src/java.base/unix/native/libjava/path_util.h line 31: > 29: int collapsible(char *names); > 30: void splitNames(char *names, char **ix); > 31: void joinNames(char *names, int nc, char **ix); Are these functions, `collapsible`, `splitNames` and `joinNames` have to be non-static? - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 13:39:46 GMT, Naoto Sato wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > The change looks reasonable. Please test your fix with macOS as well. Hi, @naotoj, Could you do me a favor to review the patch? - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v2]
On Thu, 9 Sep 2021 08:25:44 GMT, Wu Yan wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > Wu Yan has updated the pull request incrementally with one additional commit > since the last revision: > > replace realpath Hi, we directly process the characters in `linkbuf` instead of realpath. And we reuse [collapse()](https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/libjava/canonicalize_md.c#L129) to process the '.' and '..' in `linkbuf`. We tested some cases about /etc/localtime, and it works well. Test cases and results are in the attachment. [testcase.log](https://github.com/openjdk/jdk/files/7135028/testcase.log) [result.log](https://github.com/openjdk/jdk/files/7135030/result.log) - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux [v2]
> Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Wu Yan has updated the pull request incrementally with one additional commit since the last revision: replace realpath - Changes: - all: https://git.openjdk.java.net/jdk/pull/5327/files - new: https://git.openjdk.java.net/jdk/pull/5327/files/9b29edfc..19cc634d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5327=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5327=00-01 Stats: 395 lines in 4 files changed: 241 ins; 145 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/5327.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5327/head:pull/5327 PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 12:38:38 GMT, Alan Bateman wrote: > I haven't come across this configuration like but changing it to use realpath > seem reasonable. Thanks, this scenario comes from our customers. > Using `realpath` instead of `readlink` will change results on systems which > use symbolic links instead of hard links to de-duplicate the timezone files. > For example, on Debian, if `UTC` is configured (`/etc/localtime` points to > `/usr/share/zoneinfo/UTC`), I think the result will be `Etc/UTC` instead of > `UTC` after this change. But I have not actually tested this. We tested it on ubuntu, it does have this kind of problem, thank you for your reminder. `realpath` doesn't seem to be suitable here, maybe we could directly process the previous `linkbuf` to remove the extra'.' and'/'. > The change looks reasonable. Please test your fix with macOS as well. Thanks, the test on macOS is passed. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 12:51:15 GMT, Florian Weimer wrote: >> Hi, >> Please help me review the change to enhance getting time zone ID from >> /etc/localtime on linux. >> >> We use `realpath` instead of `readlink` to obtain the link name of >> /etc/localtime, because `readlink` can only read the value of a symbolic of >> link, not the canonicalized absolute pathname. >> >> For example, the value of /etc/localtime is >> "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by >> `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of >> `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which >> consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you >> can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. >> >> Thanks, >> wuyan > > src/java.base/unix/native/libjava/TimeZone_md.c line 292: > >> 290: /* canonicalize the path */ >> 291: char resolvedpath[PATH_MAX + 1]; >> 292: char *path = realpath(DEFAULT_ZONEINFO_FILE, resolvedpath); > > You really should use `realpath` with `NULL` as the second argument, to avoid > any risk of buffer overflow. Future C library headers may warn about non-null > arguments here. Ok, Thanks. I will refer to the implementation of `os::Posix::realpath` to fix it if we still use `realpath`. https://github.com/openjdk/jdk/blob/5245c1cf0260a78ca5f8ab4e7d13107f21faf071/src/hotspot/os/posix/os_posix.cpp#L805 - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 06:45:26 GMT, Wu Yan wrote: > Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan The change looks reasonable. Please test your fix with macOS as well. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 06:45:26 GMT, Wu Yan wrote: > Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan Using `realpath` instead of `readlink` will change results on systems which use symbolic links instead of hard links to de-duplicate the timezone files. For example, on Debian, if `UTC` is configured (`/etc/localtime` points to `/usr/share/zoneinfo/UTC`), I think the result will be `Etc/UTC` instead of `UTC` after this change. But I have not actually tested this. src/java.base/unix/native/libjava/TimeZone_md.c line 292: > 290: /* canonicalize the path */ > 291: char resolvedpath[PATH_MAX + 1]; > 292: char *path = realpath(DEFAULT_ZONEINFO_FILE, resolvedpath); You really should use `realpath` with `NULL` as the second argument, to avoid any risk of buffer overflow. Future C library headers may warn about non-null arguments here. - PR: https://git.openjdk.java.net/jdk/pull/5327
Re: RFR: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
On Wed, 1 Sep 2021 06:45:26 GMT, Wu Yan wrote: > Hi, > Please help me review the change to enhance getting time zone ID from > /etc/localtime on linux. > > We use `realpath` instead of `readlink` to obtain the link name of > /etc/localtime, because `readlink` can only read the value of a symbolic of > link, not the canonicalized absolute pathname. > > For example, the value of /etc/localtime is > "../usr/share/zoneinfo//Asia/Shanghai", then the linkbuf obtained by > `readlink` is "../usr/share/zoneinfo//Asia/Shanghai", and then the call of > `getZoneName(linkbuf)` will get "/Asia/Shanghai", not "Asia/Shanghai", which > consider as invalid in `ZoneInfoFile.getZoneInfo()`. Using `realpath`, you > can get “/usr/share/zoneinfo/Asia/Shanghai“ directly from “/etc/localtime“. > > Thanks, > wuyan I haven't come across this configuration like but changing it to use realpath seem reasonable. - PR: https://git.openjdk.java.net/jdk/pull/5327
