From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:50:05
This series is against 'next', and fixes the problem on Windows introduced
by commit 3ba7a06 ("A loose object is not corrupt if it cannot be read due
to EMFILE").
It unifies the dirent-emulation in compat/mingw.[ch] and compat/msvc.c,
giving us a custom implementation of opendir, readdir and closedir that
does not incorrectly set errno to 0.
Erik Faye-Lund (6):
msvc: opendir: use xmalloc
msvc: opendir: allocate enough memory
msvc: opendir: do not start the search
win32: dirent: handle errors
msvc: opendir: handle paths ending with a slash
win32: use our own dirent.h
Makefile | 7 ++-
compat/mingw.c | 60 ------------------
compat/mingw.h | 29 ---------
compat/msvc.c | 29 ---------
compat/vcbuild/include/dirent.h | 128 ---------------------------------------
compat/win32/dirent.c | 105 ++++++++++++++++++++++++++++++++
compat/win32/dirent.h | 24 +++++++
7 files changed, 134 insertions(+), 248 deletions(-)
delete mode 100644 compat/vcbuild/include/dirent.h
create mode 100644 compat/win32/dirent.c
create mode 100644 compat/win32/dirent.h
--
1.7.3.2.493.ge4bf7
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:50:05
The defintion of DIR expects the allocating function to extend
dd_name by over-allocating. This is not currently done in our
implementation of opendir. Fix this.
Signed-off-by: Erik Faye-Lund <redacted>
---
compat/msvc.c | 9 ++++-----
1 files changed, 4 insertions(+), 5 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:50:05
Previously all error conditions were ignored. Be nice, and set errno
when we should.
Signed-off-by: Erik Faye-Lund <redacted>
---
compat/mingw.c | 2 +-
compat/msvc.c | 28 +++++++++++++++++++++++++++-
2 files changed, 28 insertions(+), 2 deletions(-)
@@ -5,8 +5,29 @@DIR*opendir(constchar*name){-intlen=strlen(p->dd_name);+DWORDattrs=GetFileAttributes(name);+intlen;DIR*p;++/* check for valid path */+if(attrs==INVALID_FILE_ATTRIBUTES){+errno=ENOENT;+returnNULL;+}++/* check if it's a directory */+if(!(attrs&FILE_ATTRIBUTE_DIRECTORY)){+errno=ENOTDIR;+returnNULL;+}++/* check that the pattern won't be too long for FindFirstFileA */+len=strlen(name);+if(len+2>=MAX_PATH){+errno=ENAMETOOLONG;+returnNULL;+}+p=xmalloc(sizeof(DIR)+len+2);memset(p,0,sizeof(DIR)+len+2);strcpy(p->dd_name,name);
@@ -18,6 +39,11 @@ DIR *opendir(const char *name)}intclosedir(DIR*dir){+if(!dir){+errno=EBADF;+return-1;+}+if(dir->dd_handle!=(long)INVALID_HANDLE_VALUE)FindClose((HANDLE)dir->dd_handle);free(dir);
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:50:05
compat/mingw.c's readdir expects to be the one that starts the search,
and if it isn't, then the first entry will be missing or incorrect.
Fix this by removing the call to _findfirst, and initializing dd_handle
to INVALID_HANDLE_VALUE.
At the same time, make sure we use FindClose instead of _findclose,
which is symmetric to readdir's FindFirstFile. Take into account that
the find-handle might already be closed by readdir.
Signed-off-by: Erik Faye-Lund <redacted>
---
compat/msvc.c | 10 +++-------
1 files changed, 3 insertions(+), 7 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:50:05
The mingw-runtime implemenation of opendir, readdir and closedir
sets errno to 0 on success, something that POSIX explicitly
forbids. 3ba7a06 ("A loose object is not corrupt if it cannot be
read due to EMFILE") introduce a dependency on this behaviour,
leading to a broken "git clone" on Windows.
compat/mingw.c contains an implementation of readdir, and
compat/msvc.c contains implementations of opendir and closedir.
Move these to compat/win32/dirent.[ch], and change to our own DIR
structure at the same time.
This provides a generic Win32-implementation of opendir, readdir
and closedir which works on both MinGW and MSVC and does not reset
errno, and as a result git clone is working again on Windows.
Signed-off-by: Erik Faye-Lund <redacted>
---
Makefile | 7 ++-
compat/mingw.c | 60 ------------------
compat/mingw.h | 29 ---------
compat/msvc.c | 49 ---------------
compat/vcbuild/include/dirent.h | 128 ---------------------------------------
compat/win32/dirent.c | 105 ++++++++++++++++++++++++++++++++
compat/win32/dirent.h | 24 +++++++
7 files changed, 134 insertions(+), 268 deletions(-)
delete mode 100644 compat/vcbuild/include/dirent.h
create mode 100644 compat/win32/dirent.c
create mode 100644 compat/win32/dirent.h
@@ -1566,63 +1566,3 @@ pid_t waitpid(pid_t pid, int *status, unsigned options)errno=EINVAL;return-1;}--#ifndef NO_MINGW_REPLACE_READDIR-/* MinGW readdir implementation to avoid extra lstats for Git */-structmingw_DIR-{-struct_finddata_tdd_dta;/* disk transfer area for this dir */-structmingw_direntdd_dir;/* Our own implementation, including d_type */-longdd_handle;/* _findnext handle */-intdd_stat;/* 0 = next entry to read is first entry, -1 = off the end, positive = 0 based index of next entry */-chardd_name[1];/* given path for dir with search pattern (struct is extended) */-};--structdirent*mingw_readdir(DIR*dir)-{-WIN32_FIND_DATAAbuf;-HANDLEhandle;-structmingw_DIR*mdir=(structmingw_DIR*)dir;--if(!dir||!dir->dd_handle){-errno=EBADF;/* No set_errno for mingw */-returnNULL;-}--if(dir->dd_handle==(long)INVALID_HANDLE_VALUE&&dir->dd_stat==0)-{-DWORDlasterr;-handle=FindFirstFileA(dir->dd_name,&buf);-lasterr=GetLastError();-dir->dd_handle=(long)handle;-if(handle==INVALID_HANDLE_VALUE&&(lasterr!=ERROR_NO_MORE_FILES)){-errno=err_win_to_posix(lasterr);-returnNULL;-}-}elseif(dir->dd_handle==(long)INVALID_HANDLE_VALUE){-returnNULL;-}elseif(!FindNextFileA((HANDLE)dir->dd_handle,&buf)){-DWORDlasterr=GetLastError();-FindClose((HANDLE)dir->dd_handle);-dir->dd_handle=(long)INVALID_HANDLE_VALUE;-/* POSIX says you shouldn't set errno when readdir can't-findanymorefiles;so,ifanothererrorweleaveitset.*/-if(lasterr!=ERROR_NO_MORE_FILES)-errno=err_win_to_posix(lasterr);-returnNULL;-}--/* We get here if `buf' contains valid data. */-strcpy(dir->dd_dir.d_name,buf.cFileName);-++dir->dd_stat;--/* Set file type, based on WIN32_FIND_DATA */-mdir->dd_dir.d_type=0;-if(buf.dwFileAttributes&FILE_ATTRIBUTE_DIRECTORY)-mdir->dd_dir.d_type|=DT_DIR;-else-mdir->dd_dir.d_type|=DT_REG;--return(structdirent*)&dir->dd_dir;-}-#endif // !NO_MINGW_REPLACE_READDIR
@@ -3,53 +3,4 @@#include<conio.h>#include"../strbuf.h"-DIR*opendir(constchar*name)-{-DWORDattrs=GetFileAttributes(name);-intlen;-DIR*p;--/* check for valid path */-if(attrs==INVALID_FILE_ATTRIBUTES){-errno=ENOENT;-returnNULL;-}--/* check if it's a directory */-if(!(attrs&FILE_ATTRIBUTE_DIRECTORY)){-errno=ENOTDIR;-returnNULL;-}--/* check that the pattern won't be too long for FindFirstFileA */-len=strlen(name);-if(is_dir_sep(name[len-1]))-len--;-if(len+2>=MAX_PATH){-errno=ENAMETOOLONG;-returnNULL;-}--p=xmalloc(sizeof(DIR)+len+2);-memset(p,0,sizeof(DIR)+len+2);-strcpy(p->dd_name,name);-p->dd_name[len]='/';-p->dd_name[len+1]='*';--p->dd_handle=(long)INVALID_HANDLE_VALUE;-returnp;-}-intclosedir(DIR*dir)-{-if(!dir){-errno=EBADF;-return-1;-}--if(dir->dd_handle!=(long)INVALID_HANDLE_VALUE)-FindClose((HANDLE)dir->dd_handle);-free(dir);-return0;-}-#include"mingw.c"
@@ -1,128 +0,0 @@-/*- * DIRENT.H (formerly DIRLIB.H)- * This file has no copyright assigned and is placed in the Public Domain.- * This file is a part of the mingw-runtime package.- *- * The mingw-runtime package and its code is distributed in the hope that it- * will be useful but WITHOUT ANY WARRANTY. ALL WARRANTIES, EXPRESSED OR- * IMPLIED ARE HEREBY DISCLAIMED. This includes but is not limited to- * warranties of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.- *- * You are free to use this package and its code without limitation.- */-#ifndef _DIRENT_H_-#define _DIRENT_H_-#include <io.h>--#define PATH_MAX 512--#define __MINGW_NOTHROW--#ifndef RC_INVOKED--#ifdef __cplusplus-extern "C" {-#endif--struct dirent-{- long d_ino; /* Always zero. */- unsigned short d_reclen; /* Always zero. */- unsigned short d_namlen; /* Length of name in d_name. */- char d_name[FILENAME_MAX]; /* File name. */-};--/*- * This is an internal data structure. Good programmers will not use it- * except as an argument to one of the functions below.- * dd_stat field is now int (was short in older versions).- */-typedef struct-{- /* disk transfer area for this dir */- struct _finddata_t dd_dta;-- /* dirent struct to return from dir (NOTE: this makes this thread- * safe as long as only one thread uses a particular DIR struct at- * a time) */- struct dirent dd_dir;-- /* _findnext handle */- long dd_handle;-- /*- * Status of search:- * 0 = not started yet (next entry to read is first entry)- * -1 = off the end- * positive = 0 based index of next entry- */- int dd_stat;-- /* given path for dir with search pattern (struct is extended) */- char dd_name[PATH_MAX+3];-} DIR;--DIR* __cdecl __MINGW_NOTHROW opendir (const char*);-struct dirent* __cdecl __MINGW_NOTHROW readdir (DIR*);-int __cdecl __MINGW_NOTHROW closedir (DIR*);-void __cdecl __MINGW_NOTHROW rewinddir (DIR*);-long __cdecl __MINGW_NOTHROW telldir (DIR*);-void __cdecl __MINGW_NOTHROW seekdir (DIR*, long);---/* wide char versions */--struct _wdirent-{- long d_ino; /* Always zero. */- unsigned short d_reclen; /* Always zero. */- unsigned short d_namlen; /* Length of name in d_name. */- wchar_t d_name[FILENAME_MAX]; /* File name. */-};--/*- * This is an internal data structure. Good programmers will not use it- * except as an argument to one of the functions below.- */-typedef struct-{- /* disk transfer area for this dir */- //struct _wfinddata_t dd_dta;-- /* dirent struct to return from dir (NOTE: this makes this thread- * safe as long as only one thread uses a particular DIR struct at- * a time) */- struct _wdirent dd_dir;-- /* _findnext handle */- long dd_handle;-- /*- * Status of search:- * 0 = not started yet (next entry to read is first entry)- * -1 = off the end- * positive = 0 based index of next entry- */- int dd_stat;-- /* given path for dir with search pattern (struct is extended) */- wchar_t dd_name[1];-} _WDIR;----_WDIR* __cdecl __MINGW_NOTHROW _wopendir (const wchar_t*);-struct _wdirent* __cdecl __MINGW_NOTHROW _wreaddir (_WDIR*);-int __cdecl __MINGW_NOTHROW _wclosedir (_WDIR*);-void __cdecl __MINGW_NOTHROW _wrewinddir (_WDIR*);-long __cdecl __MINGW_NOTHROW _wtelldir (_WDIR*);-void __cdecl __MINGW_NOTHROW _wseekdir (_WDIR*, long);---#ifdef __cplusplus-}-#endif--#endif /* Not RC_INVOKED */--#endif /* Not _DIRENT_H_ */
@@ -0,0 +1,105 @@+#include"../git-compat-util.h"+#include"dirent.h"++structDIR{+structdirentdd_dir;/* includes d_type */+HANDLEdd_handle;/* FindFirstFile handle */+intdd_stat;/* 0-based index */+chardd_name[1];/* extend struct */+};++DIR*opendir(constchar*name)+{+DWORDattrs=GetFileAttributesA(name);+intlen;+DIR*p;++/* check for valid path */+if(attrs==INVALID_FILE_ATTRIBUTES){+errno=ENOENT;+returnNULL;+}++/* check if it's a directory */+if(!(attrs&FILE_ATTRIBUTE_DIRECTORY)){+errno=ENOTDIR;+returnNULL;+}++/* check that the pattern won't be too long for FindFirstFileA */+len=strlen(name);+if(is_dir_sep(name[len-1]))+len--;+if(len+2>=MAX_PATH){+errno=ENAMETOOLONG;+returnNULL;+}++p=xmalloc(sizeof(DIR)+len+2);+memset(p,0,sizeof(DIR)+len+2);+strcpy(p->dd_name,name);+p->dd_name[len]='/';+p->dd_name[len+1]='*';++p->dd_handle=INVALID_HANDLE_VALUE;+returnp;+}++structdirent*readdir(DIR*dir)+{+WIN32_FIND_DATAAbuf;+HANDLEhandle;++if(!dir||!dir->dd_handle){+errno=EBADF;/* No set_errno for mingw */+returnNULL;+}++if(dir->dd_handle==INVALID_HANDLE_VALUE&&dir->dd_stat==0){+DWORDlasterr;+handle=FindFirstFileA(dir->dd_name,&buf);+lasterr=GetLastError();+dir->dd_handle=handle;+if(handle==INVALID_HANDLE_VALUE&&(lasterr!=ERROR_NO_MORE_FILES)){+errno=err_win_to_posix(lasterr);+returnNULL;+}+}elseif(dir->dd_handle==INVALID_HANDLE_VALUE){+returnNULL;+}elseif(!FindNextFileA(dir->dd_handle,&buf)){+DWORDlasterr=GetLastError();+FindClose(dir->dd_handle);+dir->dd_handle=INVALID_HANDLE_VALUE;+/* POSIX says you shouldn't set errno when readdir can't+findanymorefiles;so,ifanothererrorweleaveitset.*/+if(lasterr!=ERROR_NO_MORE_FILES)+errno=err_win_to_posix(lasterr);+returnNULL;+}++/* We get here if `buf' contains valid data. */+strcpy(dir->dd_dir.d_name,buf.cFileName);+++dir->dd_stat;++/* Set file type, based on WIN32_FIND_DATA */+dir->dd_dir.d_type=0;+if(buf.dwFileAttributes&FILE_ATTRIBUTE_DIRECTORY)+dir->dd_dir.d_type|=DT_DIR;+else+dir->dd_dir.d_type|=DT_REG;++return&dir->dd_dir;+}++intclosedir(DIR*dir)+{+if(!dir){+errno=EBADF;+return-1;+}++if(dir->dd_handle!=INVALID_HANDLE_VALUE)+FindClose(dir->dd_handle);+free(dir);+return0;+}
@@ -23,6 +23,8 @@ DIR *opendir(const char *name)/* check that the pattern won't be too long for FindFirstFileA */len=strlen(name);+if(is_dir_sep(name[len-1]))+len--;if(len+2>=MAX_PATH){errno=ENAMETOOLONG;returnNULL;
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:05
Erik Faye-Lund wrote:
quoted hunk
--- a/compat/msvc.c+++ b/compat/msvc.c
@@ -7,16 +7,13 @@ DIR *opendir(const char *name){intlen;DIR*p;-p=(DIR*)malloc(sizeof(DIR));+p=xmalloc(sizeof(DIR));memset(p,0,sizeof(DIR));strncpy(p->dd_name,name,PATH_MAX);len=strlen(p->dd_name);p->dd_name[len]='/';p->dd_name[len+1]='*';-if(p==NULL)-returnNULL;
A behavior change but maybe a good one. For example, the
prune_packed_objects() loop currently skips object dirs it can't open,
even if that is due to memory exhaustion, but this changes it to error
out.
What is the motivation?
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:05
Erik Faye-Lund wrote:
quoted hunk
--- a/compat/msvc.c+++ b/compat/msvc.c
@@ -23,6 +23,8 @@ DIR *opendir(const char *name)/* check that the pattern won't be too long for FindFirstFileA */len=strlen(name);+if(is_dir_sep(name[len-1]))+len--;
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:05
Erik Faye-Lund wrote:
This provides a generic Win32-implementation of opendir, readdir
and closedir which works on both MinGW and MSVC and does not reset
errno, and as a result git clone is working again on Windows.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:50:05
On Tue, Nov 23, 2010 at 6:40 PM, Jonathan Nieder [off-list ref] wrote:
Erik Faye-Lund wrote:
quoted
--- a/compat/msvc.c+++ b/compat/msvc.c
@@ -7,16 +7,13 @@ DIR *opendir(const char *name)
{
int len;
DIR *p;
- p = (DIR*)malloc(sizeof(DIR));
+ p = xmalloc(sizeof(DIR));
memset(p, 0, sizeof(DIR));
strncpy(p->dd_name, name, PATH_MAX);
len = strlen(p->dd_name);
p->dd_name[len] = '/';
p->dd_name[len+1] = '*';
- if (p == NULL)
- return NULL;
A behavior change but maybe a good one. For example, the
prune_packed_objects() loop currently skips object dirs it can't open,
even if that is due to memory exhaustion, but this changes it to error
out.
What is the motivation?
The motivation is just to avoid having to deal with the error, like we
do other places. It's not a big deal though. I could also set errno to
ENOMEM and return NULL if that's preferable. I just don't see how it
is.
I also slightly dislike setting an error not listed in POSIX'
documentation of opendir, even though it's probably allowed.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:50:05
On Tue, Nov 23, 2010 at 6:45 PM, Jonathan Nieder [off-list ref] wrote:
Erik Faye-Lund wrote:
quoted
This provides a generic Win32-implementation of opendir, readdir
and closedir which works on both MinGW and MSVC and does not reset
errno, and as a result git clone is working again on Windows.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:50:05
On Tue, Nov 23, 2010 at 6:45 PM, Erik Faye-Lund [off-list ref] wrote:
On Tue, Nov 23, 2010 at 6:40 PM, Jonathan Nieder [off-list ref] wrote:
quoted
Erik Faye-Lund wrote:
quoted
--- a/compat/msvc.c+++ b/compat/msvc.c
@@ -7,16 +7,13 @@ DIR *opendir(const char *name)
{
int len;
DIR *p;
- p = (DIR*)malloc(sizeof(DIR));
+ p = xmalloc(sizeof(DIR));
memset(p, 0, sizeof(DIR));
strncpy(p->dd_name, name, PATH_MAX);
len = strlen(p->dd_name);
p->dd_name[len] = '/';
p->dd_name[len+1] = '*';
- if (p == NULL)
- return NULL;
A behavior change but maybe a good one. For example, the
prune_packed_objects() loop currently skips object dirs it can't open,
even if that is due to memory exhaustion, but this changes it to error
out.
What is the motivation?
The motivation is just to avoid having to deal with the error, like we
do other places. It's not a big deal though. I could also set errno to
ENOMEM and return NULL if that's preferable. I just don't see how it
is.
I also slightly dislike setting an error not listed in POSIX'
documentation of opendir, even though it's probably allowed.
Of course, I don't have to set errno; malloc would already have done
that. All I need to do is move the check for NULL a bit earlier, so it
won't segfault on ENOMEM. I'll change it for the next round.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:05
Erik Faye-Lund wrote:
The motivation is just to avoid having to deal with the error, like we
do other places. It's not a big deal though. I could also set errno to
ENOMEM and return NULL if that's preferable. I just don't see how it
is.
I don't disagree; just fishing for a commit message. :)
I also slightly dislike setting an error not listed in POSIX'
documentation of opendir, even though it's probably allowed.
For future reference, here's what POSIX has to say.
Implementations shall not generate a different error
number from one required by this volume of
POSIX.1-2008 for an error condition described in this
volume of POSIX.1-2008, but may generate additional
errors unless explicitly disallowed for a particular
function.
So ENOMEM would have been allowed from that front.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:50:05
On Tue, Nov 23, 2010 at 7:02 PM, Jonathan Nieder [off-list ref] wrote:
Erik Faye-Lund wrote:
quoted
The motivation is just to avoid having to deal with the error, like we
do other places. It's not a big deal though. I could also set errno to
ENOMEM and return NULL if that's preferable. I just don't see how it
is.
I don't disagree; just fishing for a commit message. :)
Yes, but after stopping to think about it a tad more, I tend to like
it working more similar to existing POSIX implementations.
I am adding a commit message this time, though! ;)
quoted
I also slightly dislike setting an error not listed in POSIX'
documentation of opendir, even though it's probably allowed.
For future reference, here's what POSIX has to say.
Implementations shall not generate a different error
number from one required by this volume of
POSIX.1-2008 for an error condition described in this
volume of POSIX.1-2008, but may generate additional
errors unless explicitly disallowed for a particular
function.
So ENOMEM would have been allowed from that front.