readdir is incorrectly implemented at Windows
Hi hackers,
Small issue with readir implementation for Windows.
Right now it returns ENOENT in case of any error returned by FindFirstFile.
So all places in Postgres where opendir/listdir are used will assume
that directory is empty and
do nothing without reporting any error.
It is not so good if directory is actually not empty but there are not
enough permissions for accessing the directory and FindFirstFile
returns ERROR_ACCESS_DENIED:
struct dirent *
readdir(DIR *d)
{
WIN32_FIND_DATA fd;
if (d->handle == INVALID_HANDLE_VALUE)
{
d->handle = FindFirstFile(d->dirname, &fd);
if (d->handle == INVALID_HANDLE_VALUE)
{
errno = ENOENT;
return NULL;
}
}
Attached please find small patch fixing the problem.
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
dirent.patchtext/x-patch; name=dirent.patchDownload
diff --git a/src/port/dirent.c b/src/port/dirent.c
index 7a91450..284bf27 100644
--- a/src/port/dirent.c
+++ b/src/port/dirent.c
@@ -83,7 +83,13 @@ readdir(DIR *d)
d->handle = FindFirstFile(d->dirname, &fd);
if (d->handle == INVALID_HANDLE_VALUE)
{
- errno = ENOENT;
+ if (GetLastError() == ERROR_FILE_NOT_FOUND)
+ {
+ /* No more files, force errno=0 (unlike mingw) */
+ errno = 0;
+ return NULL;
+ }
+ _dosmaperr(GetLastError());
return NULL;
}
}
Originally bug was reported by Yuri Kurenkov:
https://github.com/postgrespro/pg_probackup/issues/48
As pg_probackup rely on readdir() for listing files to backup, wrong
permissions could lead to a broken backup.
On 02/25/2019 06:38 PM, Konstantin Knizhnik wrote:
Hi hackers,
Small issue with readir implementation for Windows.
Right now it returns ENOENT in case of any error returned by
FindFirstFile.
So all places in Postgres where opendir/listdir are used will assume
that directory is empty and
do nothing without reporting any error.
It is not so good if directory is actually not empty but there are not
enough permissions for accessing the directory and FindFirstFile
returns ERROR_ACCESS_DENIED:struct dirent *
readdir(DIR *d)
{
WIN32_FIND_DATA fd;if (d->handle == INVALID_HANDLE_VALUE)
{
d->handle = FindFirstFile(d->dirname, &fd);
if (d->handle == INVALID_HANDLE_VALUE)
{
errno = ENOENT;
return NULL;
}
}Attached please find small patch fixing the problem.
--
Grigory Smolkin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Mon, Feb 25, 2019 at 06:38:16PM +0300, Konstantin Knizhnik wrote:
Small issue with readir implementation for Windows.
Right now it returns ENOENT in case of any error returned by FindFirstFile.
So all places in Postgres where opendir/listdir are used will assume that
directory is empty and
do nothing without reporting any error.
It is not so good if directory is actually not empty but there are not
enough permissions for accessing the directory and FindFirstFile
returns ERROR_ACCESS_DENIED:
Yeah, I think that you are right here. We should have a clean error
if permissions are incorrect, and your patch does what is mentioned on
Windows docs:
https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-findfirstfilea
Could you add it to the next commit fest as a bug fix please? I think
that I will be able to look at that in details soon, but if not it
would be better to not lose track of your fix.
--
Michael
On Thu, Feb 28, 2019 at 11:15:53AM +0900, Michael Paquier wrote:
Could you add it to the next commit fest as a bug fix please? I think
that I will be able to look at that in details soon, but if not it
would be better to not lose track of your fix.
Okay, I have looked at your patch. And double-checked on Windows. To
put it short, I agree with the approach you are taking. I have been
curious about the mention to MinGW in the existing code as well as in
the patch you are proposing, and I have checked if what your patch and
what the current state are correct, and I think that HEAD is wrong on
one thing.
First mingw64 code can be found here:
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/
https://github.com/Alexpux/mingw-w64/
Then, the implementation of readdir/opendir/closedir can be found in
mingw-w64-crt/misc/dirent.c. Looking at their implementation of
readdir, I can see two things:
1) When beginning a search in a directory, _tfindfirst gets used,
which returns ENOENT as error if no matches are found:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/findfirst-functions?view=vs-2017
So from this point of view your patch is right: you make readdir()
return errno=0 which matches the normal *nix behaviors. And MinGW
does not do that.
2) On follow-up lookups, MinGW code uses _tfindnext, and actually
*enforces* errno=0 when seeing ERROR_NO_MORE_FILES. So from this
point of view the code's comment in HEAD is incorrect as of today.
The current implementation exists since 399a36a so perhaps MinGW was
not like that when dirent.c has been added in src/port/, but that's
not true today. So let's fix the comment at the same time.
Attached is an updated patch with my suggestions. Does it look fine
to you?
Also, I think that we should credit Yuri Kurenkov for the discovery of
the issue, with yourself, Konstantin, as the author of the patch.
Are there other people involved which should be credited? Like
Grigory?
--
Michael
Attachments:
readdir-win32-fix.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/port/dirent.c b/src/port/dirent.c
index 7a91450695..191fd062a5 100644
--- a/src/port/dirent.c
+++ b/src/port/dirent.c
@@ -83,7 +83,13 @@ readdir(DIR *d)
d->handle = FindFirstFile(d->dirname, &fd);
if (d->handle == INVALID_HANDLE_VALUE)
{
- errno = ENOENT;
+ if (GetLastError() == ERROR_FILE_NOT_FOUND)
+ {
+ /* No files at all, force errno=0 (unlike mingw) */
+ errno = 0;
+ return NULL;
+ }
+ _dosmaperr(GetLastError());
return NULL;
}
}
@@ -93,7 +99,7 @@ readdir(DIR *d)
{
if (GetLastError() == ERROR_NO_MORE_FILES)
{
- /* No more files, force errno=0 (unlike mingw) */
+ /* No more files, force errno=0 (like mingw) */
errno = 0;
return NULL;
}
On 01.03.2019 9:13, Michael Paquier wrote:
On Thu, Feb 28, 2019 at 11:15:53AM +0900, Michael Paquier wrote:
Could you add it to the next commit fest as a bug fix please? I think
that I will be able to look at that in details soon, but if not it
would be better to not lose track of your fix.Okay, I have looked at your patch. And double-checked on Windows. To
put it short, I agree with the approach you are taking. I have been
curious about the mention to MinGW in the existing code as well as in
the patch you are proposing, and I have checked if what your patch and
what the current state are correct, and I think that HEAD is wrong on
one thing.First mingw64 code can be found here:
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/
https://github.com/Alexpux/mingw-w64/Then, the implementation of readdir/opendir/closedir can be found in
mingw-w64-crt/misc/dirent.c. Looking at their implementation of
readdir, I can see two things:
1) When beginning a search in a directory, _tfindfirst gets used,
which returns ENOENT as error if no matches are found:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/findfirst-functions?view=vs-2017
So from this point of view your patch is right: you make readdir()
return errno=0 which matches the normal *nix behaviors. And MinGW
does not do that.
2) On follow-up lookups, MinGW code uses _tfindnext, and actually
*enforces* errno=0 when seeing ERROR_NO_MORE_FILES. So from this
point of view the code's comment in HEAD is incorrect as of today.
The current implementation exists since 399a36a so perhaps MinGW was
not like that when dirent.c has been added in src/port/, but that's
not true today. So let's fix the comment at the same time.Attached is an updated patch with my suggestions. Does it look fine
to you?
Yes, certainly.
Also, I think that we should credit Yuri Kurenkov for the discovery of
the issue, with yourself, Konstantin, as the author of the patch.
Are there other people involved which should be credited? Like
Grigory?
Yes,� Yuri Kurenkov and Grigory Smalking did a lot in investigation of
this problem.
(the irony is that the problem detected by Yuri was caused by another
bug in pg_probackup, but we thought
that it was related with permissions and come to this issue).
--
Michael
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Fri, Mar 01, 2019 at 10:23:02AM +0300, Konstantin Knizhnik wrote:
Yes, Yuri Kurenkov and Grigory Smalking did a lot in investigation
of this problem.
(the irony is that the problem detected by Yuri was caused by
another bug in pg_probackup, but we thought that it was related with
permissions and come to this issue).
Thanks for confirming, Konstantin. Let's wait a couple of days to see
if anybody has objections or comments, and I'll try to commit this
patch.
--
Michael
On 2/25/19 10:38 AM, Konstantin Knizhnik wrote:
Hi hackers,
Small issue with readir implementation for Windows.
Right now it returns ENOENT in case of any error returned by
FindFirstFile.
So all places in Postgres where opendir/listdir are used will assume
that directory is empty and
do nothing without reporting any error.
It is not so good if directory is actually not empty but there are not
enough permissions for accessing the directory and FindFirstFile
returns ERROR_ACCESS_DENIED:struct dirent *
readdir(DIR *d)
{
WIN32_FIND_DATA fd;if (d->handle == INVALID_HANDLE_VALUE)
{
d->handle = FindFirstFile(d->dirname, &fd);
if (d->handle == INVALID_HANDLE_VALUE)
{
errno = ENOENT;
return NULL;
}
}Attached please find small patch fixing the problem.
Diagnosis seems correct. I wonder if this is responsible for some odd
things we've seen over time on Windows.
This reads a bit oddly:
{
- errno = ENOENT;
+ if (GetLastError() == ERROR_FILE_NOT_FOUND)
+ {
+ /* No more files, force errno=0 (unlike mingw) */
+ errno = 0;
+ return NULL;
+ }
+ _dosmaperr(GetLastError());
return NULL;
}Why not something like:
if (GetLastError() == ERROR_FILE_NOT_FOUND)
errno = 0;
else
_dosmaperr(GetLastError());
return NULL;
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Mar 01, 2019 at 04:43:13PM +0900, Michael Paquier wrote:
Thanks for confirming, Konstantin. Let's wait a couple of days to see
if anybody has objections or comments, and I'll try to commit this
patch.
Done and backpatched down to 9.4, with Andrew's suggestion from
upthread included.
--
Michael