Hi, On 14-11-16 23:45, David Sommerseth wrote: > Commit 825e2ec1f358f2e8 cleaned up the usage of > warn_if_group_others_accessible() > and moved it into options.c. At this point there is only one caller of this > function, check_file_access(). > > This takes that clean-up one step further and merges everything into > check_file_access(). In addition it removes some no longer needed #ifdefs > and uses platform_stat() to allow a similar check to happen on the Windows > platform as well. > > Signed-off-by: David Sommerseth <[email protected]> > --- > src/openvpn/options.c | 38 ++++++++++++-------------------------- > 1 file changed, 12 insertions(+), 26 deletions(-) > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index 4d31e4c..369f3f4 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -57,6 +57,7 @@ > #include "manage.h" > #include "forward.h" > #include "ssl_verify.h" > +#include "platform.h" > #include <ctype.h> > > #include "memdbg.h" > @@ -2683,31 +2684,6 @@ options_postprocess_mutate (struct options *o) > */ > #ifndef ENABLE_SMALL /** Expect people using the stripped down version to > know what they do */ > > -/* > - * Warn if a given file is group/others accessible. > - */ > -static void > -warn_if_group_others_accessible (const char* filename) > -{ > -#ifndef WIN32 > -#ifdef HAVE_STAT > - if (strcmp (filename, INLINE_FILE_TAG)) > - { > - struct stat st; > - if (stat (filename, &st)) > - { > - msg (M_WARN | M_ERRNO, "WARNING: cannot stat file '%s'", filename); > - } > - else > - { > - if (st.st_mode & (S_IRWXG|S_IRWXO)) > - msg (M_WARN, "WARNING: file '%s' is group or others accessible", > filename); > - } > - } > -#endif > -#endif > -} > - > #define CHKACC_FILE (1<<0) /** Check for a file/directory precense */ > #define CHKACC_DIRPATH (1<<1) /** Check for directory precense where a > file should reside */ > #define CHKACC_FILEXSTWR (1<<2) /** If file exists, is it writable? */ > @@ -2754,9 +2730,19 @@ check_file_access(const int type, const char *file, > const int mode, const char * > if (platform_access (file, W_OK) != 0) > errcode = errno; > > + /* Warn if a given file is group/others accessible. */ > if (type & CHKACC_PRIVATE) > { > - warn_if_group_others_accessible (file); > + platform_stat_t st; > + if (platform_stat (file, &st)) > + { > + msg (M_WARN | M_ERRNO, "WARNING: cannot stat file '%s'", file); > + } > + else > + { > + if (st.st_mode & (S_IRWXG|S_IRWXO)) > + msg (M_WARN, "WARNING: file '%s' is group or others accessible", > file); > + } > } > > /* Scream if an error is found */ >
Hm, I like the removal of the #ifdefs, but I don't think we should merge warn_if_group_others_accessible() into check_file_access(). Why? Because when it was a separate function, the code was self-documenting and nicely separated. Now, we need an extra comment that basically exists of the previous function name to explain what we're doing. I was also wondering, have you tested that this work as expected on Windows? Since Windows has a quite different permission system. Or will is simply never warn, like before? That would be fine for this patch. -Steffan ------------------------------------------------------------------------------ _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
