On 16-11-16 15:59, David Sommerseth wrote: > On 16/11/16 11:44, Steffan Karger wrote: >> 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? > > That function is already fairly short, and these additional lines in > the function doesn't add much. And it falls perfectly within the > scope of "check file access". Had the scope for this function been > broader and expected to be used more places, it would have a place as > a separate function, IMO. > >> 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. > > The overall change reduces the lines of code with 14 lines. Just 4 of > them are #ifdef related. And I personally find it easier to read a > comment and then see instantly what happens below. Otherwise you read > a function name, guess what it does based on its name but don't know > before you've located the function and read it. > > So when there's a benefit of reducing the total amount of lines, I > value that even more. > > With that said, there are plenty of reasons why not to merge > functions. But from my point of view, the use scope for > warn_if_group_others_accessible() is clearly defined and falls > perfectly under the responsibility check_file_access() primarily > should have. And with just one caller, I think this approach is > simpler and cleaner.
We clearly have different opinions here, so I guess we'll have to discuss that further some time. But as far as I'm concerned that should not hold up this patch. >> 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. > > I do not have any Windows test environments, so I'll admit that's a > bold (and risky) move of me. However, as you've identified, > S_IRWXG|S_IRWXO is not set so shouldn't cause any issues. Combined with a compile-test I just this and the fact that the worst thing that could happen is an incorrect warning (Windows builds never warned before), I feel confident enough to ACK this: ACK. -Steffan
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
