The Samba-Bugzilla – Attachment 8743 Details for
Bug 9777
vfs_dirsort uses non-stackable calls, dirfd(), malloc instead of talloc and doesn't cope with directories being modified whilst reading.
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for master and 4.0.next.
vfs_dirsort.patchset (text/plain), 17.45 KB, created by
Jeremy Allison
on 2013-04-10 00:00:26 UTC
(
hide
)
Description:
git-am fix for master and 4.0.next.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2013-04-10 00:00:26 UTC
Size:
17.45 KB
patch
obsolete
>From 36158965c7f0c7fcbf1f3aaf264a553d7f1fa650 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 8 Apr 2013 15:11:28 -0700 >Subject: [PATCH 01/10] Change source3/modules/vfs_dirsort.c from MALLOC -> > TALLOC. > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/modules/vfs_dirsort.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) > >diff --git a/source3/modules/vfs_dirsort.c b/source3/modules/vfs_dirsort.c >index 98472f8..8139556 100644 >--- a/source3/modules/vfs_dirsort.c >+++ b/source3/modules/vfs_dirsort.c >@@ -37,10 +37,7 @@ struct dirsort_privates { > }; > > static void free_dirsort_privates(void **datap) { >- struct dirsort_privates *data = (struct dirsort_privates *) *datap; >- SAFE_FREE(data->directory_list); >- SAFE_FREE(data); >- *datap = NULL; >+ TALLOC_FREE(*datap); > } > > static bool open_and_sort_dir (vfs_handle_struct *handle) >@@ -69,9 +66,10 @@ static bool open_and_sort_dir (vfs_handle_struct *handle) > SMB_VFS_NEXT_REWINDDIR(handle, data->source_directory); > > /* Set up an array and read the directory entries into it */ >- SAFE_FREE(data->directory_list); /* destroy previous cache if needed */ >- data->directory_list = (struct dirent *)SMB_MALLOC( >- data->number_of_entries * sizeof(struct dirent)); >+ TALLOC_FREE(data->directory_list); /* destroy previous cache if needed */ >+ data->directory_list = talloc_zero_array(data, >+ struct dirent, >+ data->number_of_entries); > if (!data->directory_list) { > return false; > } >@@ -95,9 +93,7 @@ static DIR *dirsort_opendir(vfs_handle_struct *handle, > struct dirsort_privates *data = NULL; > > /* set up our private data about this directory */ >- data = (struct dirsort_privates *)SMB_MALLOC( >- sizeof(struct dirsort_privates)); >- >+ data = talloc_zero(handle->conn, struct dirsort_privates); > if (!data) { > return NULL; > } >@@ -130,9 +126,7 @@ static DIR *dirsort_fdopendir(vfs_handle_struct *handle, > struct dirsort_privates *data = NULL; > > /* set up our private data about this directory */ >- data = (struct dirsort_privates *)SMB_MALLOC( >- sizeof(struct dirsort_privates)); >- >+ data = talloc_zero(handle->conn, struct dirsort_privates); > if (!data) { > return NULL; > } >@@ -145,7 +139,7 @@ static DIR *dirsort_fdopendir(vfs_handle_struct *handle, > attr); > > if (data->source_directory == NULL) { >- SAFE_FREE(data); >+ TALLOC_FREE(data); > return NULL; > } > >-- >1.8.1.3 > > >From 0217d5c69a2aeae302dc1aef84c7dda160c989fd Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 8 Apr 2013 16:31:53 -0700 >Subject: [PATCH 02/10] Protect against early error in SMB_VFS_NEXT_READDIR. > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/modules/vfs_dirsort.c | 4 ++++ > 1 file changed, 4 insertions(+) > >diff --git a/source3/modules/vfs_dirsort.c b/source3/modules/vfs_dirsort.c >index 8139556..6fe7c18 100644 >--- a/source3/modules/vfs_dirsort.c >+++ b/source3/modules/vfs_dirsort.c >@@ -61,6 +61,10 @@ static bool open_and_sort_dir (vfs_handle_struct *handle) > data->number_of_entries++; > } > >+ if (data->number_of_entries == 0) { >+ return false; >+ } >+ > /* Open the underlying directory and count the number of entries > Skip back to the beginning as we'll read it again */ > SMB_VFS_NEXT_REWINDDIR(handle, data->source_directory); >-- >1.8.1.3 > > >From bac1a12bcf71557f9f48b647677be2d19f5991c5 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 8 Apr 2013 16:38:03 -0700 >Subject: [PATCH 03/10] Use an index i rather than re-using a state variable. > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/modules/vfs_dirsort.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > >diff --git a/source3/modules/vfs_dirsort.c b/source3/modules/vfs_dirsort.c >index 6fe7c18..1c24bc9 100644 >--- a/source3/modules/vfs_dirsort.c >+++ b/source3/modules/vfs_dirsort.c >@@ -44,7 +44,7 @@ static bool open_and_sort_dir (vfs_handle_struct *handle) > { > struct dirent *dp; > struct stat dir_stat; >- long current_pos; >+ unsigned int i; > struct dirsort_privates *data = NULL; > > SMB_VFS_HANDLE_GET_DATA(handle, data, struct dirsort_privates, >@@ -77,15 +77,13 @@ static bool open_and_sort_dir (vfs_handle_struct *handle) > if (!data->directory_list) { > return false; > } >- current_pos = data->pos; >- data->pos = 0; >+ i = 0; > while ((dp = SMB_VFS_NEXT_READDIR(handle, data->source_directory, > NULL)) != NULL) { >- data->directory_list[data->pos++] = *dp; >+ data->directory_list[i++] = *dp; > } > > /* Sort the directory entries by name */ >- data->pos = current_pos; > TYPESAFE_QSORT(data->directory_list, data->number_of_entries, compare_dirent); > return true; > } >-- >1.8.1.3 > > >From 1560479e7622268ef54c4521d8c74899f1a81663 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 8 Apr 2013 16:40:35 -0700 >Subject: [PATCH 04/10] Protect open_and_sort_dir() from the directory changing > size. > >Otherwise there could be an error between initial count, allocation >and re-read. > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/modules/vfs_dirsort.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > >diff --git a/source3/modules/vfs_dirsort.c b/source3/modules/vfs_dirsort.c >index 1c24bc9..3ea720f 100644 >--- a/source3/modules/vfs_dirsort.c >+++ b/source3/modules/vfs_dirsort.c >@@ -30,7 +30,7 @@ static int compare_dirent (const struct dirent *da, const struct dirent *db) > struct dirsort_privates { > long pos; > struct dirent *directory_list; >- long number_of_entries; >+ unsigned int number_of_entries; > time_t mtime; > DIR *source_directory; > int fd; >@@ -42,9 +42,9 @@ static void free_dirsort_privates(void **datap) { > > static bool open_and_sort_dir (vfs_handle_struct *handle) > { >- struct dirent *dp; > struct stat dir_stat; > unsigned int i; >+ unsigned int total_count = 0; > struct dirsort_privates *data = NULL; > > SMB_VFS_HANDLE_GET_DATA(handle, data, struct dirsort_privates, >@@ -58,10 +58,10 @@ static bool open_and_sort_dir (vfs_handle_struct *handle) > > while (SMB_VFS_NEXT_READDIR(handle, data->source_directory, NULL) > != NULL) { >- data->number_of_entries++; >+ total_count++; > } > >- if (data->number_of_entries == 0) { >+ if (total_count == 0) { > return false; > } > >@@ -73,16 +73,22 @@ static bool open_and_sort_dir (vfs_handle_struct *handle) > TALLOC_FREE(data->directory_list); /* destroy previous cache if needed */ > data->directory_list = talloc_zero_array(data, > struct dirent, >- data->number_of_entries); >+ total_count); > if (!data->directory_list) { > return false; > } >- i = 0; >- while ((dp = SMB_VFS_NEXT_READDIR(handle, data->source_directory, >- NULL)) != NULL) { >- data->directory_list[i++] = *dp; >+ for (i = 0; i < total_count; i++) { >+ struct dirent *dp = SMB_VFS_NEXT_READDIR(handle, >+ data->source_directory, >+ NULL); >+ if (dp == NULL) { >+ break; >+ } >+ data->directory_list[i] = *dp; > } > >+ data->number_of_entries = i; >+ > /* Sort the directory entries by name */ > TYPESAFE_QSORT(data->directory_list, data->number_of_entries, compare_dirent); > return true; >-- >1.8.1.3 > > >From 1c98a9c2e523144f9e7c403ab4077dbda6e1a2ff Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 9 Apr 2013 10:29:47 -0700 >Subject: [PATCH 05/10] Clean error paths in opendir and fd_opendir by only > setting handle data on success. > >Pass extra struct dirsort_privates * to open_and_sort_dir() function >to avoid it having to re-read the handle data. > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/modules/vfs_dirsort.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > >diff --git a/source3/modules/vfs_dirsort.c b/source3/modules/vfs_dirsort.c >index 3ea720f..d6f19b5 100644 >--- a/source3/modules/vfs_dirsort.c >+++ b/source3/modules/vfs_dirsort.c >@@ -40,7 +40,8 @@ static void free_dirsort_privates(void **datap) { > TALLOC_FREE(*datap); > } > >-static bool open_and_sort_dir (vfs_handle_struct *handle) >+static bool open_and_sort_dir(vfs_handle_struct *handle, >+ struct dirsort_privates *data) > { > struct stat dir_stat; > unsigned int i; >@@ -115,14 +116,15 @@ static DIR *dirsort_opendir(vfs_handle_struct *handle, > > data->fd = dirfd(data->source_directory); > >- SMB_VFS_HANDLE_SET_DATA(handle, data, free_dirsort_privates, >- struct dirsort_privates, return NULL); >- >- if (!open_and_sort_dir(handle)) { >+ if (!open_and_sort_dir(handle, data)) { > SMB_VFS_NEXT_CLOSEDIR(handle,data->source_directory); >+ TALLOC_FREE(data); > return NULL; > } > >+ SMB_VFS_HANDLE_SET_DATA(handle, data, free_dirsort_privates, >+ struct dirsort_privates, return NULL); >+ > return data->source_directory; > } > >@@ -153,16 +155,17 @@ static DIR *dirsort_fdopendir(vfs_handle_struct *handle, > > data->fd = dirfd(data->source_directory); > >- SMB_VFS_HANDLE_SET_DATA(handle, data, free_dirsort_privates, >- struct dirsort_privates, return NULL); >- >- if (!open_and_sort_dir(handle)) { >+ if (!open_and_sort_dir(handle, data)) { > SMB_VFS_NEXT_CLOSEDIR(handle,data->source_directory); >+ TALLOC_FREE(data); > /* fd is now closed. */ > fsp->fh->fd = -1; > return NULL; > } > >+ SMB_VFS_HANDLE_SET_DATA(handle, data, free_dirsort_privates, >+ struct dirsort_privates, return NULL); >+ > return data->source_directory; > } > >@@ -185,7 +188,7 @@ static struct dirent *dirsort_readdir(vfs_handle_struct *handle, > > /* throw away cache and re-read the directory if we've changed */ > if (current_mtime > data->mtime) { >- open_and_sort_dir(handle); >+ open_and_sort_dir(handle, data); > } > > if (data->pos >= data->number_of_entries) { >-- >1.8.1.3 > > >From fbe27563a56b5ecae294e8bd0c667d0f1243284c Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 9 Apr 2013 10:38:24 -0700 >Subject: [PATCH 06/10] Check SMB_VFS_NEXT_OPENDIR return in dirsort_opendir(). > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/modules/vfs_dirsort.c | 5 +++++ > 1 file changed, 5 insertions(+) > >diff --git a/source3/modules/vfs_dirsort.c b/source3/modules/vfs_dirsort.c >index d6f19b5..e2c61da 100644 >--- a/source3/modules/vfs_dirsort.c >+++ b/source3/modules/vfs_dirsort.c >@@ -114,6 +114,11 @@ static DIR *dirsort_opendir(vfs_handle_struct *handle, > data->source_directory = SMB_VFS_NEXT_OPENDIR(handle, fname, mask, > attr); > >+ if (data->source_directory == NULL) { >+ TALLOC_FREE(data); >+ return NULL; >+ } >+ > data->fd = dirfd(data->source_directory); > > if (!open_and_sort_dir(handle, data)) { >-- >1.8.1.3 > > >From d294f86347039d7a4eab57f0bd5e5125dd9d1aa5 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 9 Apr 2013 10:43:53 -0700 >Subject: [PATCH 07/10] Convert mtime from a time_t to a struct timespec. > >In preparation for removing the dirfd and using fsp_stat() >and VFS_STAT functions. > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/modules/vfs_dirsort.c | 41 ++++++++++++++++++++++++++--------------- > 1 file changed, 26 insertions(+), 15 deletions(-) > >diff --git a/source3/modules/vfs_dirsort.c b/source3/modules/vfs_dirsort.c >index e2c61da..3388d53 100644 >--- a/source3/modules/vfs_dirsort.c >+++ b/source3/modules/vfs_dirsort.c >@@ -31,7 +31,7 @@ struct dirsort_privates { > long pos; > struct dirent *directory_list; > unsigned int number_of_entries; >- time_t mtime; >+ struct timespec mtime; > DIR *source_directory; > int fd; > }; >@@ -40,21 +40,35 @@ static void free_dirsort_privates(void **datap) { > TALLOC_FREE(*datap); > } > >+static bool get_sorted_dir_mtime(vfs_handle_struct *handle, >+ struct dirsort_privates *data, >+ struct timespec *ret_mtime) >+{ >+ int ret; >+ struct stat dir_stat; >+ >+ ret = fstat(data->fd, &dir_stat); >+ >+ if (ret == -1) { >+ return false; >+ } >+ >+ ret_mtime->tv_sec = dir_stat.st_mtime; >+ ret_mtime->tv_nsec = 0; >+ >+ return true; >+} >+ > static bool open_and_sort_dir(vfs_handle_struct *handle, > struct dirsort_privates *data) > { >- struct stat dir_stat; >- unsigned int i; >+ unsigned int i = 0; > unsigned int total_count = 0; >- struct dirsort_privates *data = NULL; >- >- SMB_VFS_HANDLE_GET_DATA(handle, data, struct dirsort_privates, >- return false); > > data->number_of_entries = 0; > >- if (fstat(data->fd, &dir_stat) == 0) { >- data->mtime = dir_stat.st_mtime; >+ if (get_sorted_dir_mtime(handle, data, &data->mtime) == false) { >+ return false; > } > > while (SMB_VFS_NEXT_READDIR(handle, data->source_directory, NULL) >@@ -179,20 +193,17 @@ static struct dirent *dirsort_readdir(vfs_handle_struct *handle, > SMB_STRUCT_STAT *sbuf) > { > struct dirsort_privates *data = NULL; >- time_t current_mtime; >- struct stat dir_stat; >+ struct timespec current_mtime; > > SMB_VFS_HANDLE_GET_DATA(handle, data, struct dirsort_privates, > return NULL); > >- if (fstat(data->fd, &dir_stat) == -1) { >+ if (get_sorted_dir_mtime(handle, data, ¤t_mtime) == false) { > return NULL; > } > >- current_mtime = dir_stat.st_mtime; >- > /* throw away cache and re-read the directory if we've changed */ >- if (current_mtime > data->mtime) { >+ if (timespec_compare(¤t_mtime, &data->mtime) > 1) { > open_and_sort_dir(handle, data); > } > >-- >1.8.1.3 > > >From e6eb2b445d01550290f388c7eb2e0ebfcb8f362e Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 9 Apr 2013 10:50:55 -0700 >Subject: [PATCH 08/10] Remove the use of dirfd inside the vfs_dirsort.c. > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/modules/vfs_dirsort.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > >diff --git a/source3/modules/vfs_dirsort.c b/source3/modules/vfs_dirsort.c >index 3388d53..4741239 100644 >--- a/source3/modules/vfs_dirsort.c >+++ b/source3/modules/vfs_dirsort.c >@@ -33,7 +33,8 @@ struct dirsort_privates { > unsigned int number_of_entries; > struct timespec mtime; > DIR *source_directory; >- int fd; >+ files_struct *fsp; /* If open via FDOPENDIR. */ >+ struct smb_filename *smb_fname; /* If open via OPENDIR */ > }; > > static void free_dirsort_privates(void **datap) { >@@ -45,16 +46,21 @@ static bool get_sorted_dir_mtime(vfs_handle_struct *handle, > struct timespec *ret_mtime) > { > int ret; >- struct stat dir_stat; >- >- ret = fstat(data->fd, &dir_stat); >+ struct timespec *pmtime; >+ >+ if (data->fsp) { >+ ret = fsp_stat(data->fsp); >+ pmtime = &data->fsp->fsp_name->st.st_ex_mtime; >+ } else { >+ ret = SMB_VFS_STAT(handle->conn, data->smb_fname); >+ pmtime = &data->smb_fname->st.st_ex_mtime; >+ } > > if (ret == -1) { > return false; > } > >- ret_mtime->tv_sec = dir_stat.st_mtime; >- ret_mtime->tv_nsec = 0; >+ *ret_mtime = *pmtime; > > return true; > } >@@ -113,6 +119,7 @@ static DIR *dirsort_opendir(vfs_handle_struct *handle, > const char *fname, const char *mask, > uint32 attr) > { >+ NTSTATUS status; > struct dirsort_privates *data = NULL; > > /* set up our private data about this directory */ >@@ -124,6 +131,16 @@ static DIR *dirsort_opendir(vfs_handle_struct *handle, > data->directory_list = NULL; > data->pos = 0; > >+ status = create_synthetic_smb_fname(data, >+ fname, >+ NULL, >+ NULL, >+ &data->smb_fname); >+ if (!NT_STATUS_IS_OK(status)) { >+ TALLOC_FREE(data); >+ return NULL; >+ } >+ > /* Open the underlying directory and count the number of entries */ > data->source_directory = SMB_VFS_NEXT_OPENDIR(handle, fname, mask, > attr); >@@ -133,8 +150,6 @@ static DIR *dirsort_opendir(vfs_handle_struct *handle, > return NULL; > } > >- data->fd = dirfd(data->source_directory); >- > if (!open_and_sort_dir(handle, data)) { > SMB_VFS_NEXT_CLOSEDIR(handle,data->source_directory); > TALLOC_FREE(data); >@@ -162,6 +177,7 @@ static DIR *dirsort_fdopendir(vfs_handle_struct *handle, > > data->directory_list = NULL; > data->pos = 0; >+ data->fsp = fsp; > > /* Open the underlying directory and count the number of entries */ > data->source_directory = SMB_VFS_NEXT_FDOPENDIR(handle, fsp, mask, >@@ -172,8 +188,6 @@ static DIR *dirsort_fdopendir(vfs_handle_struct *handle, > return NULL; > } > >- data->fd = dirfd(data->source_directory); >- > if (!open_and_sort_dir(handle, data)) { > SMB_VFS_NEXT_CLOSEDIR(handle,data->source_directory); > TALLOC_FREE(data); >-- >1.8.1.3 > > >From 24304180816a8f1bc80759ad27fe6d88ca4db6bf Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 9 Apr 2013 11:02:58 -0700 >Subject: [PATCH 09/10] Remove unneeded initializations (we already > talloc_zero). > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/modules/vfs_dirsort.c | 5 ----- > 1 file changed, 5 deletions(-) > >diff --git a/source3/modules/vfs_dirsort.c b/source3/modules/vfs_dirsort.c >index 4741239..adb2614 100644 >--- a/source3/modules/vfs_dirsort.c >+++ b/source3/modules/vfs_dirsort.c >@@ -128,9 +128,6 @@ static DIR *dirsort_opendir(vfs_handle_struct *handle, > return NULL; > } > >- data->directory_list = NULL; >- data->pos = 0; >- > status = create_synthetic_smb_fname(data, > fname, > NULL, >@@ -175,8 +172,6 @@ static DIR *dirsort_fdopendir(vfs_handle_struct *handle, > return NULL; > } > >- data->directory_list = NULL; >- data->pos = 0; > data->fsp = fsp; > > /* Open the underlying directory and count the number of entries */ >-- >1.8.1.3 > > >From 25ee52dac928fbeb7f2151b60d01dfcdcf76af7a Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 9 Apr 2013 16:56:24 -0700 >Subject: [PATCH 10/10] Ensure we test the dirsort module in make test. > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > selftest/target/Samba3.pm | 1 + > 1 file changed, 1 insertion(+) > >diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm >index 72c1116..1b14f1c 100755 >--- a/selftest/target/Samba3.pm >+++ b/selftest/target/Samba3.pm >@@ -1026,6 +1026,7 @@ sub provision($$$$$$) > path = $shrdir > comment = encrypt smb username is [%U] > smb encrypt = required >+ vfs objects = $vfs_modulesdir_abs/dirsort.so > [tmpguest] > path = $shrdir > guest ok = yes >-- >1.8.1.3 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Actions:
View
Attachments on
bug 9777
:
8743
|
8761
|
8962
|
8963