Re: Revisiting Re: BUG #8532: postgres fails to start with timezone-data >=2013e - Mailing list pgsql-hackers

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 11/04/15 05:30 PM, Tom Lane wrote:
> Ian Stakenvicius <axs@gentoo.org> writes:
>> Hey all -- so I know that Gentoo Linux is likely the only
>> platform this bug occurs under, but i got annoyed enough with it
>> that I decided to write a patch to fix this issue once and for
>> all (or at least, help keep it from happening).
> 
>> That thread in question actually dealt with crashing on startup
>> in postgresql-9.1 and earlier, but all versions including the
>> latest still suffer from the inability to load timezone data via
>> the pg_timezone_* tables if /usr/share/zoneinfo contains
>> filesystem loops.
> 
>> To that end, the following helps resolve this issue by ensuring 
>> single-level filesystem loops are detected and skipped.  The top
>> half of the patch only applies to postgresql-9.1 and earlier,
>> while the second half applies to all versions.
> 
> This patch doesn't look right at all.  In the first place, tzdirsub
> is the entire subpath, not necessarily just one filename.  In the
> second place, limiting the strncmp comparison to
> strlen(direntry->d_name) exposes you to false matches --- for
> example, a current d_name of "foo" would be thought to match a
> tzdirsub path of "foobar".  In the third place, relying on 
> basename(1) does not seem advisable for portability reasons ---
> use something from src/port/path.c instead.
> 


That would be due to a misinterpretation of tzdirsub on my part, then
; inspecting the code it had seemed to me that for each level of
recursion, tzdirsub is incremented so that it only contains the target
working directory rather than the entire subpath.

Yes, false matches are also a possibility when the directory being
tested is a substring of the parent directory, however based on the
format of zoneinfo it seems unlikely this will occur -- the closest
example we have is "America/Indiana/Indianapolis" , except that to
trigger the false positive it would have to be structured as
"America/Indianapolis/Indiana"; it was a risk and certainly could be
further mitigated via strcmp() instead or some other check.

Basename is posix (and i used it in the manner necessary for the posix
implementation rather than the gnu implementation), so it should be
available on most platforms, but certainly a different implementation
could be used to obtain the top directory instead of stripping it from
the path via basename.


> It would probably be a good idea to be more specific about what
> sorts of loops you are hoping to catch, because this surely won't
> detect all possible loops as-is.


My original goal was to simply stop recursion when the 'posix' subdir
of /usr/share/zoneinfo is a symlink to the current directory (ie:
'posix' -> '.') -- the first iteration should still be processed (ie,
posix and all subdirs), but posix/posix would not be processed.

I wanted to do it generically though so that it could prevent such
recursion on at least a subset of possible filesystem loops.

I know that this code doesn't detect multi-level filesystem loops (ie:
'posix/something' -> '..') and that the code necessary to detect that
would need to be much more complicated.  Also I felt that if any
filesystem loop were to exist within /usr/share/zoneinfo, likely it
would be a single-level loop such as the 'posix' -> '.' one I'm
dealing with on Gentoo.


Thanks for the feedback; I'll look into adjusting the patch and post
back a better version in the future.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iF4EAREIAAYFAlUrzHAACgkQ2ugaI38ACPD78AD/bxjL6YSrOVxwlufLF0BeJmXT
wrkPUxUdG0i6UA8GoiABALGiepu/ZLhdv/80pOapmBOZmaCygChX9dIsgXpqtIuE
=5/QU
-----END PGP SIGNATURE-----