Unset TZDIR will cause crash without /usr/share/zoneinfo

[scdub]

When using ORC on a platform that doesn't have /usr/share/zoneinfo defined nor the TZDIR environment variable set, the library will crash. Specifically on Windows, the DEFAULT_TZDIR location is an invalid path, but even on Linux the file is not present on minimal images like the default alpine container. It looks like the TZDATA is downloaded during the build and is referenced during the C++ tests of ORC, but this change won't affect the runtime DLLs. Perhaps a relative location or option to configure a TZDATA location would make sense? In my case working with conda, I patched the routine to locate the tzdata that is included within the conda environment, but that wouldn't be applicable to all use cases.

[dongjoon-hyun]

Thank you for reporting, @scdub . Do you mean ORC C++ library or ORC Java library?

[scdub]

@dongjoon-hyun Sorry for not being more specific — the crash happens in the ORC C++ library. When the readFile call is made in getTimezoneByFilename, a crash will occur on Windows where /usr/share/zoneinfo/UTC is an invalid path:

[dongjoon-hyun]

Thank you for the details, @scdub .

cc @wgtmac

[wgtmac]

Sure, let me take a look.

[wgtmac]

@scdub After a quick check, it seems that either TZDIR or /usr/share/zoneinfo to be set is a must to run the ORC C++ library. From the WIN32 test, it downloads TZDATA and sets TZDIR=${TZDATA_DIR}. Could you set TZDIR to point to TZDATA in the enviroment?

[scdub]

@wgtmac Yes, I can confirm it will work if the TZDIR is set and extracted, as is the case with the tests themselves. However, the TZDATA isn't included in the steps for building the distribution, and relying on an environment variable has other challenges. I believe that the code should provide a clear error message rather than crash if the current design is intended. From my perspective it would be nice to not be reliant on a POSIX time interface or include one in the distribution output with instructions about the requirement to configure TZDIR or ensure the host operation system provides /usr/share/zoneinfo for e.g. barebones Docker containers.

[wgtmac]

It would be a lot of maintenance burden if the ORC library includes a TZDB in its distribution output. I prefer providing a better error message when the local tzdb is not found.

BTW, C++20 has standardized std::chrono::tzdb to represent a copy of the IANA TZDB. As we are still on C++17 yet, we can switch to it once we make the C++20 transition.

[wgtmac]

Proposed fix on the exception message: #1587

[dongjoon-hyun]

I hope the proposed fix can mitigate the situation in Apache ORC 2.0.0.