Re: [HACKERS] Revisiting Re: BUG #8532: postgres fails to start with timezone-data =2013e
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 On 11/04/15 05:30 PM, Tom Lane wrote: Ian Stakenvicius a...@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- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Revisiting Re: BUG #8532: postgres fails to start with timezone-data =2013e
Ian Stakenvicius a...@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. 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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Revisiting Re: BUG #8532: postgres fails to start with timezone-data =2013e
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. (hopefully the patch attached properly) Regards, Ian --- a/src/timezone/pgtz.c 2015-02-02 15:45:23.0 -0500 +++ b/src/timezone/pgtz.c 2015-04-07 14:21:22.341832190 -0400 @@ -586,6 +586,12 @@ if (direntry-d_name[0] == '.') continue; + /* if current working directory has the same name as current direntry name, + * then skip as this is a recursive fs loop + */ + if (strncmp(direntry-d_name,tzdirsub,strlen(direntry-d_name)) == 0) + continue; + snprintf(tzdir + tzdir_orig_len, MAXPGPATH - tzdir_orig_len, /%s, direntry-d_name); @@ -1615,6 +1621,13 @@ if (direntry-d_name[0] == '.') continue; + /* copy current working directory so that there is no risk of modification by basename(), + * and compare to current direntry name; skip if they are the same as this is a recursive fs loop + */ + snprintf(fullname, MAXPGPATH, %s, dir-dirname[dir-depth]); + if (strncmp(direntry-d_name,basename(fullname),strlen(direntry-d_name)) == 0) + continue; + snprintf(fullname, MAXPGPATH, %s/%s, dir-dirname[dir-depth], direntry-d_name); if (stat(fullname, statbuf) != 0) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
