file checks: Merge warn_if_group_others_accessible() into check_file_access()

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(-)

src/openvpn/options.c +5 -26

index 4d31e4c..369f3f4 100644
@@ -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))
+	{

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.

Hi,

Quoted 50 lines · David Sommerseth

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(-)

src/openvpn/options.c index renamed from src/openvpn/options.c +0 -0

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 */

src/openvpn/options.c index renamed from src/openvpn/options.c +0 -2

-/* - * 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 - {

src/openvpn/options.c index renamed from src/openvpn/options.c +0 -1

-	  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); + } }

Quoted 2 lines

/* 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.

Quoted 51 lines · Steffan Karger

Quoted 50 lines · David Sommerseth

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(-)

src/openvpn/options.c index renamed from src/openvpn/options.c +0 -0

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 */

src/openvpn/options.c index renamed from src/openvpn/options.c +0 -2

-/* - * 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 - {

src/openvpn/options.c index renamed from src/openvpn/options.c +0 -1

-	  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); + } }

Quoted 6 lines

Quoted 2 lines

/* 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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 4 lines · Steffan Karger

Quoted 3 lines

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.

Quoted 5 lines

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.

But the question is will this compile for windows? I doubt it will unless we add some dummy defines for S_IRWXG etc..