Re: [PATCH xserver v2 1/4] os: move xf86PrivsElevated here

2018-03-14 Thread Nicolai Hähnle 

On 14.03.2018 14:36, Emil Velikov wrote:

On 13 March 2018 at 21:46, Ben Crocker  wrote:


--- a/include/os.h
+++ b/include/os.h
@@ -368,6 +368,9 @@ System(const char *cmdline);
  #define Fclose(a) fclose(a)
  #endif

+extern _X_EXPORT Bool
+PrivsElevated(void);
+

Any particular reason why this is exported?
Is it simply mimicking the surrounding code, or there's a genuine reason for it?


I believe I just mimicked the surrounding code, but it's been a while...

Cheers,
Nicolai



Not an issue either way, the series is
Reviewed-by: Emil Velikov 

Somewhat unrelated:
Seems like commit 49f77fff1495c0a2050fb18f9b1fc627839bbfc2 exported
1000+ symbols as part of (?) folding the extension modules in core.
Yet it never followed to hide the internal functions as things were done.

-Emil



___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver v2 1/4] os: move xf86PrivsElevated here

2018-03-14 Thread Emil Velikov 
On 13 March 2018 at 21:46, Ben Crocker  wrote:

> --- a/include/os.h
> +++ b/include/os.h
> @@ -368,6 +368,9 @@ System(const char *cmdline);
>  #define Fclose(a) fclose(a)
>  #endif
>
> +extern _X_EXPORT Bool
> +PrivsElevated(void);
> +
Any particular reason why this is exported?
Is it simply mimicking the surrounding code, or there's a genuine reason for it?

Not an issue either way, the series is
Reviewed-by: Emil Velikov 

Somewhat unrelated:
Seems like commit 49f77fff1495c0a2050fb18f9b1fc627839bbfc2 exported
1000+ symbols as part of (?) folding the extension modules in core.
Yet it never followed to hide the internal functions as things were done.

-Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver v2 1/4] os: move xf86PrivsElevated here

2018-03-13 Thread Ben Crocker 
From: Nicolai Hähnle 

Having different types of code all trying to check for elevated privileges
is a bad idea. This implementation is the most thorough one.

Signed-off-by: Nicolai Hähnle 
Reviewed-by: Ben Crocker 
Reviewed-by: Antoine Martin 
Tested-by: Ben Crocker 
---
 hw/xfree86/common/xf86Init.c | 59 +
 include/os.h |  3 +++
 os/utils.c   | 63 
 3 files changed, 67 insertions(+), 58 deletions(-)

diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
index cdbf80c61..88d202463 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -238,64 +238,7 @@ xf86PrintBanner(void)
 Bool
 xf86PrivsElevated(void)
 {
-static Bool privsTested = FALSE;
-static Bool privsElevated = TRUE;
-
-if (!privsTested) {
-#if defined(WIN32)
-privsElevated = FALSE;
-#else
-if ((getuid() != geteuid()) || (getgid() != getegid())) {
-privsElevated = TRUE;
-}
-else {
-#if defined(HAVE_ISSETUGID)
-privsElevated = issetugid();
-#elif defined(HAVE_GETRESUID)
-uid_t ruid, euid, suid;
-gid_t rgid, egid, sgid;
-
-if ((getresuid(, , ) == 0) &&
-(getresgid(, , ) == 0)) {
-privsElevated = (euid != suid) || (egid != sgid);
-}
-else {
-printf("Failed getresuid or getresgid");
-/* Something went wrong, make defensive assumption */
-privsElevated = TRUE;
-}
-#else
-if (getuid() == 0) {
-/* running as root: uid==euid==0 */
-privsElevated = FALSE;
-}
-else {
-/*
- * If there are saved ID's the process might still be 
privileged
- * even though the above test succeeded. If issetugid() and
- * getresgid() aren't available, test this by trying to set
- * euid to 0.
- */
-unsigned int oldeuid;
-
-oldeuid = geteuid();
-
-if (seteuid(0) != 0) {
-privsElevated = FALSE;
-}
-else {
-if (seteuid(oldeuid) != 0) {
-FatalError("Failed to drop privileges.  Exiting\n");
-}
-privsElevated = TRUE;
-}
-}
-#endif
-}
-#endif
-privsTested = TRUE;
-}
-return privsElevated;
+return PrivsElevated();
 }
 
 static void
diff --git a/include/os.h b/include/os.h
index e141a6b02..7b4cb9481 100644
--- a/include/os.h
+++ b/include/os.h
@@ -368,6 +368,9 @@ System(const char *cmdline);
 #define Fclose(a) fclose(a)
 #endif
 
+extern _X_EXPORT Bool
+PrivsElevated(void);
+
 extern _X_EXPORT void
 CheckUserParameters(int argc, char **argv, char **envp);
 extern _X_EXPORT void
diff --git a/os/utils.c b/os/utils.c
index 4a8d1249f..4305dab26 100644
--- a/os/utils.c
+++ b/os/utils.c
@@ -1719,6 +1719,69 @@ System(const char *cmdline)
 }
 #endif
 
+Bool
+PrivsElevated(void)
+{
+static Bool privsTested = FALSE;
+static Bool privsElevated = TRUE;
+
+if (!privsTested) {
+#if defined(WIN32)
+privsElevated = FALSE;
+#else
+if ((getuid() != geteuid()) || (getgid() != getegid())) {
+privsElevated = TRUE;
+}
+else {
+#if defined(HAVE_ISSETUGID)
+privsElevated = issetugid();
+#elif defined(HAVE_GETRESUID)
+uid_t ruid, euid, suid;
+gid_t rgid, egid, sgid;
+
+if ((getresuid(, , ) == 0) &&
+(getresgid(, , ) == 0)) {
+privsElevated = (euid != suid) || (egid != sgid);
+}
+else {
+printf("Failed getresuid or getresgid");
+/* Something went wrong, make defensive assumption */
+privsElevated = TRUE;
+}
+#else
+if (getuid() == 0) {
+/* running as root: uid==euid==0 */
+privsElevated = FALSE;
+}
+else {
+/*
+ * If there are saved ID's the process might still be 
privileged
+ * even though the above test succeeded. If issetugid() and
+ * getresgid() aren't available, test this by trying to set
+ * euid to 0.
+ */
+unsigned int oldeuid;
+
+oldeuid = geteuid();
+
+if (seteuid(0) != 0) {
+privsElevated = FALSE;
+}
+else {
+if (seteuid(oldeuid) != 0) {
+FatalError("Failed to drop privileges.  Exiting\n");
+