Re: [hackers] [sbase] [PATCH 1/5] Remove st != NULL checks from recursor functions
Hi Laslo On Tue, Dec 27, 2016 at 02:59:56PM +0100, Laslo Hunhold wrote: > On Wed, 14 Dec 2016 19:40:02 -0800 > Michael Forney wrote: > > Hey Michael, > > > In the description of 3111908b034c73673a2f079b2b13a88c18379baa, it > > says that the functions must be able to handle st being NULL, but > > recurse always passes a valid pointer. The only function that was > > ever passed NULL was rm(), but this was changed to go through recurse > > in 2f4ab527391135e651b256f8654b050ea4a48f3d, so now the checks are > > pointless. > > have you tested this patchset extensively? I hate to admit that the > recursor-subsystem is probably the most fragile part of sbase and > really need more feedback on these patches by more people (Silvan, have > you had the chance to test this?). Sadly, I have not :/ I just did some very basic testing using the following directory structure. testdir/ testdir/testdir2 testdir/testdir2/testB.txt testdir/testdir2/testA.txt testdir/link (symbolic link) testdir/test1.txt testdir/test2.txt I used the recursive options of tar, chgrp, chmod, chown, du and rm after applying Michael's patch on these files/directories and everything worked as expected. Ideally, somebody that uses sbase in a development system regularly should apply and test the patches further. Cheers, Silvan > Cheers > > Laslo > > -- > Laslo Hunhold >
Re: [hackers] [sbase] [PATCH 1/5] Remove st != NULL checks from recursor functions
On 12/27/16, Laslo Hunhold wrote: > Hey Michael, > >> In the description of 3111908b034c73673a2f079b2b13a88c18379baa, it >> says that the functions must be able to handle st being NULL, but >> recurse always passes a valid pointer. The only function that was >> ever passed NULL was rm(), but this was changed to go through recurse >> in 2f4ab527391135e651b256f8654b050ea4a48f3d, so now the checks are >> pointless. > > have you tested this patchset extensively? I hate to admit that the > recursor-subsystem is probably the most fragile part of sbase and > really need more feedback on these patches by more people (Silvan, have > you had the chance to test this?). I have been running with these patches since I posted them to the list and have not run into any issues. Here is some analysis that shows that it is safe: recurse is called in - chgrp.c with fn = chgrp - chmod.c with fn = chmodr - chown.c with fn = chownpwgr - du.c with fn = du - mv.c with fn = rm - rm.c with fn = rm - tar.c with fn = c recurse calls fn with `(r->fn)(path, &st, data, r)` (in three locations) and `(r->fn)(path, &dst, data, r)`. In all cases, st is the address of a struct st. chgrp, chmodr, chownpwgr, du, rm, and c are all only called through recurse.
Re: [hackers] [sbase] [PATCH 1/5] Remove st != NULL checks from recursor functions
On Wed, 14 Dec 2016 19:40:02 -0800 Michael Forney wrote: Hey Michael, > In the description of 3111908b034c73673a2f079b2b13a88c18379baa, it > says that the functions must be able to handle st being NULL, but > recurse always passes a valid pointer. The only function that was > ever passed NULL was rm(), but this was changed to go through recurse > in 2f4ab527391135e651b256f8654b050ea4a48f3d, so now the checks are > pointless. have you tested this patchset extensively? I hate to admit that the recursor-subsystem is probably the most fragile part of sbase and really need more feedback on these patches by more people (Silvan, have you had the chance to test this?). Cheers Laslo -- Laslo Hunhold
[hackers] [sbase] [PATCH 1/5] Remove st != NULL checks from recursor functions
In the description of 3111908b034c73673a2f079b2b13a88c18379baa, it says
that the functions must be able to handle st being NULL, but recurse
always passes a valid pointer. The only function that was ever passed
NULL was rm(), but this was changed to go through recurse in
2f4ab527391135e651b256f8654b050ea4a48f3d, so now the checks are
pointless.
---
chgrp.c | 4 ++--
chmod.c | 4 ++--
chown.c | 2 +-
du.c | 6 +++---
libutil/rm.c | 2 +-
tar.c| 2 +-
6 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/chgrp.c b/chgrp.c
index 299238b..5d3a71b 100644
--- a/chgrp.c
+++ b/chgrp.c
@@ -26,10 +26,10 @@ chgrp(const char *path, struct stat *st, void *data, struct
recursor *r)
chownf = chown;
}
- if (st && chownf(path, st->st_uid, gid) < 0) {
+ if (chownf(path, st->st_uid, gid) < 0) {
weprintf("%s %s:", chownf_name, path);
ret = 1;
- } else if (st && S_ISDIR(st->st_mode)) {
+ } else if (S_ISDIR(st->st_mode)) {
recurse(path, NULL, r);
}
}
diff --git a/chmod.c b/chmod.c
index a0d96f1..109a7e5 100644
--- a/chmod.c
+++ b/chmod.c
@@ -13,11 +13,11 @@ chmodr(const char *path, struct stat *st, void *data,
struct recursor *r)
{
mode_t m;
- m = parsemode(modestr, st ? st->st_mode : 0, mask);
+ m = parsemode(modestr, st->st_mode, mask);
if (chmod(path, m) < 0) {
weprintf("chmod %s:", path);
ret = 1;
- } else if (st && S_ISDIR(st->st_mode)) {
+ } else if (S_ISDIR(st->st_mode)) {
recurse(path, NULL, r);
}
}
diff --git a/chown.c b/chown.c
index 2009507..d7dc8e0 100644
--- a/chown.c
+++ b/chown.c
@@ -32,7 +32,7 @@ chownpwgr(const char *path, struct stat *st, void *data,
struct recursor *r)
if (chownf(path, uid, gid) < 0) {
weprintf("%s %s:", chownf_name, path);
ret = 1;
- } else if (st && S_ISDIR(st->st_mode)) {
+ } else if (S_ISDIR(st->st_mode)) {
recurse(path, NULL, r);
}
}
diff --git a/du.c b/du.c
index 3dc3545..fb2c90b 100644
--- a/du.c
+++ b/du.c
@@ -39,11 +39,11 @@ du(const char *path, struct stat *st, void *total, struct
recursor *r)
{
off_t subtotal = 0;
- if (st && S_ISDIR(st->st_mode))
+ if (S_ISDIR(st->st_mode))
recurse(path, &subtotal, r);
- *((off_t *)total) += subtotal + nblks(st ? st->st_blocks : 0);
+ *((off_t *)total) += subtotal + nblks(st->st_blocks);
- if (!sflag && r->depth <= maxdepth && r->depth && st &&
(S_ISDIR(st->st_mode) || aflag))
+ if (!sflag && r->depth <= maxdepth && r->depth && (S_ISDIR(st->st_mode)
|| aflag))
printpath(subtotal + nblks(st->st_blocks), path);
}
diff --git a/libutil/rm.c b/libutil/rm.c
index f6a54b6..11c20fb 100644
--- a/libutil/rm.c
+++ b/libutil/rm.c
@@ -13,7 +13,7 @@ int rm_status = 0;
void
rm(const char *path, struct stat *st, void *data, struct recursor *r)
{
- if (!r->maxdepth && st && S_ISDIR(st->st_mode)) {
+ if (!r->maxdepth && S_ISDIR(st->st_mode)) {
recurse(path, NULL, r);
if (rmdir(path) < 0) {
diff --git a/tar.c b/tar.c
index 71719b0..d2161d4 100644
--- a/tar.c
+++ b/tar.c
@@ -370,7 +370,7 @@ c(const char *path, struct stat *st, void *data, struct
recursor *r)
if (vflag)
puts(path);
- if (st && S_ISDIR(st->st_mode))
+ if (S_ISDIR(st->st_mode))
recurse(path, NULL, r);
}
--
2.11.0
