[PATCH 0/3] 64 Bit Fixes.

[Ian....@cern.ch]

I've resubmited two of my previous patches that contain fixes for compilation
failures on 64 bit boxes, a third patch fixes this issue for include_server_if.c

Thanks

Ian.

[Ian....@cern.ch]

[Ian....@cern.ch]

[Ian....@cern.ch]

[Fergus Henderson]

Generally this is looking pretty good, and my only major concern is that this patch needs tests.
I think you have addressed that with a separate patch, but please make sure that this patch is not committed until the tests are ready to be committed too.

I have some detailed comments below.

On Thu, Mar 5, 2009 at 11:14 AM, <Ian....@cern.ch> wrote:

+int recv_token(int sd, gss_buffer_t token) {
+    int ret;
+    uint32_t length;
+
+    if ((ret = dcc_r_token_int(sd, "TLEN", &length)) != 0) {

length should have type "unsigned", not "uint32_t", otherwise this will result in a compile error on platforms where "uint32_t" is not the same type as "unsigned".

+    token->value = malloc(length);
+
+    if (token->value == NULL && length != 0) {
+       rs_log_error("malloc failed : %lu bytes: %s.",
+                                       (unsigned long) length,
+                                       strerror(errno));

It's probably not worth including strerror(errno) here.  At best, if the malloc implementation follows the Unix98 standard, errno will always be ENOMEM at this point, in which case you might as well hard-code the string "out of memory".
At worst, the malloc implementation might not set errno at all, and then the output could be misleading.

===================================================================
--- quilt-distcc-3.1.orig/distcc-3.1/src/exitcode.h
+++ quilt-distcc-3.1/distcc-3.1/src/exitcode.h
@@ -59,7 +59,18 @@ enum dcc_exitcode {
    EXIT_NO_SUCH_FILE             = 115,
    EXIT_NO_HOSTS                 = 116,
    EXIT_GONE                     = 117, /**< No longer relevant */
+#ifdef HAVE_GSSAPI
+    EXIT_TIMEOUT                  = 118,
+    EXIT_IMPORT_NAME_ERROR        = 119, /**< GSS-API - Failed to import name to internal format*/
+    EXIT_FAILED_TO_INIT_SEC_CXT   = 120, /**< GSS-API - Client failed to initiate a secure context*/
+    EXIT_FAILED_TO_ACCEPT_SEC_CXT = 121, /**< GSS-API - Server failed to accept a secure context*/
+    EXIT_FLAG_MISMATCH_ERROR      = 122, /**< GSS-API - Requested flags don't match those returned/supported*/
+    EXIT_ACQUIRE_CREDS_FAILED    = 123, /**< GSS-API - Failed to acquire principal credentials for Server*/
+    EXIT_NO_PEER_AUTH            = 124, /**< GSS-API - Peer not providing authentication handshake*/
+    EXIT_NO_PRINCIPAL_NAME       = 125  /**< GSS-API - No principal name specified for Server*/

Hmm... this list doesn't match the list in the man page... see below.

 

Index: quilt-distcc-3.1/distcc-3.1/src/serve.c
===================================================================
--- quilt-distcc-3.1.orig/distcc-3.1/src/serve.c
+++ quilt-distcc-3.1/distcc-3.1/src/serve.c
@@ -90,12 +90,22 @@
 #include "stringmap.h"
 #include "dotd.h"
 #include "fix_debug_info.h"
+#ifdef HAVE_GSSAPI
+#include "auth.h"
+
+/*Global security context in case confidentiality/integrity*/
+/*services are needed in the future.*/
+extern gss_ctx_id_t distccd_ctx_handle;
+#endif

 /**
 * We copy all serious distccd messages to this file, as well as sending the
 * compiler errors there, so they're visible to the client.
 **/
 static int dcc_compile_log_fd = -1;
+#ifdef HAVE_GSSAPI
+int dcc_auth_enabled = 0;
+#endif

Can you move that above dcc_compile_log_fd and its comment, and make sure that there's a blank line separating them?

Also, can you add a comment explaining the meaning of this int?
(E.g. "boolean; non-zero iff the DISTCC_AUTH environment variable is set.", or something along those lines.)

Index: quilt-distcc-3.1/distcc-3.1/man/distcc.1
@@ -691,6 +726,19 @@ No hosts defined and fallbacks disabled.
 .TP
 118
 Timeout.
+.TP
+119
+GSS-API - Failed to import name to internal format.
+.TP
+120
+GSS-API - Client failed to initiate a secure context.
+.TP
+122
+GSS-API - Requested flags don't match those returned/supported.
+.TP
+124
+GSS-API - Peer not providing authentication handshake.

This list doesn't match the list in the header file. 123 and 125 are missing.
Do those only occur for distccd?
 

+.TP
+.B "DISTCC_AUTH"
+If set to 1, peform GSS-API based mutual authentication.
+.B This option is only available if distcc was compiled with
+.B the --with-auth configure option.
+.TP
+.B "DISTCC_PRINCIPAL"
+If set, specifies the name of the principal that distccd runs under, and is used
+to authenticate the server to the client.

Can you add

.B This environment variable is only used if distcc was compiled with
.B the --with-auth configure option and the DISTCC_AUTH environment variable is set to 1

please?  Or other wording with appropriate conditions?

Index: quilt-distcc-3.1/distcc-3.1/man/distccd.1
===================================================================
--- quilt-distcc-3.1.orig/distcc-3.1/man/distccd.1
+++ quilt-distcc-3.1/distcc-3.1/man/distccd.1
@@ -217,6 +217,15 @@ name or IP address in their distcc host
 just use "+zeroconf" in their distcc host lists.
 .B This option is only available if distccd was compiled with
 .B Avahi support enabled.
+.TP
+.B --auth
+Peform GSS-API based mutual authentication.
+.B This option is only available if distccd was compiled with
+.B the --with-auth configure option.
+.TP
+.B --show-principal
+Displays the name of the distccd security principal extracted from the
+environment.

Can you add

.B This option is only available if distccd was compiled with
.B the --with-auth configure option.

please?

BTW, what happens if you specify --show-principle without --auth?  Does that work?
 

@@ -278,6 +287,10 @@ On Linux, turn on the TCP_DEFER_ACCEPT s
 .B "TMPDIR"
 Directory for temporary files such as preprocessor output.  By default
 /tmp/ is used.
+.TP
+.B "DISTCCD_PRINCIPAL"
+If set, specifies the name of the principal that distccd runs under, and is used
+to authenticate with the client.

Can you add

+.B This environment variable is only used if distccd was compiled with
+.B the --with-auth configure option and if distccd is run with the --auth option.

please?  Or appropriate wording that explains when this is used.

--
Fergus Henderson <fer...@google.com>

[Fergus Henderson]

Looks good to me!  Thanks.

Do you want to go ahead and commit this patch?

If so, go to <http://code.google.com/p/distcc/source/checkout>,
sign in (you may need create a google account if you don't already have one),
follow the directions on how to check out the source if you plan to make changes,
apply the patch, and then "svn commit".

On Thu, Mar 5, 2009 at 11:14 AM, <Ian....@cern.ch> wrote:

This patch fixes a portability issue that causes
compilation to fail on 64 bit boxes.


Index: quilt-distcc-3.1/distcc-3.1/src/include_server_if.c
===================================================================
--- quilt-distcc-3.1.orig/distcc-3.1/src/include_server_if.c
+++ quilt-distcc-3.1/distcc-3.1/src/include_server_if.c 

@@ -85,8 +85,8 @@ int dcc_talk_to_include_server(char **ar
    }

    if (strlen(include_server_port) >= ((int)sizeof(sa.sun_path) - 1)) {
-        rs_log_warning("$INCLUDE_SERVER_PORT is longer than %d characters",
-                       (sizeof(sa.sun_path) - 1));
+        rs_log_warning("$INCLUDE_SERVER_PORT is longer than %ld characters",
+                       ((long) sizeof(sa.sun_path) - 1));
        return 1;
    }

--
Fergus Henderson <fer...@google.com>

[Fergus Henderson]

On Thu, Mar 5, 2009 at 11:14 AM, <Ian....@cern.ch> wrote:
>

> +static int dcc_gssapi_check_list(char *principal, int sd) {
> + char *pos = NULL;
> + int location, mode, ret;
> +
> + if (opt_blacklist_enabled) {
> + mode = 1;
> + } else {
> + mode = 0;
> + }
...
> + if (mode) { /*blacklist*/

Why not just use

if (opt_blacklist_enabled)

rather going to the trouble of setting and using this "mode" local variable?

> +/*
> + * Perform a binary search on a sorted list for a key.
> + *
> + * @param key. The search key.
> + *
> + * Returns index if key in list, otherwise
> + * KEY_NOT_FOUND condition.
> + */
> +static int dcc_gssapi_bin_search(char *key) {
> + int bottom = 0;
> + int middle;
> + int top = list_count - 1;
> +
> + while (bottom <= top) {
> + middle = (bottom + top) / 2;
> +
> + if (strcmp(key, list[middle]) < 0) {
> + top = middle - 1;
> + } else if (strcmp(key, list[middle]) > 0) {

You should call strcmp() once and save the result in a local variable,
rather than calling strcmp() twice.

> + if (!(file = fopen(arg_list_file, "w"))) {

Hmm, why are we writing to the arg_list_file there?

> +static int dcc_gssapi_compare_strings(const void *string_one,
> + const void *string_two) {
> + const char **s1 = (const char **) string_one;
> + const char **s2 = (const char **) string_two;
> +
> + return strncmp(*s1, *s2, MAX_NAME_LENGTH);

Why strncmp() here rather than strcmp()?
Are the principal names not guaranteed to be null-terminated?

--
Fergus Henderson <fergus.h...@gmail.com>

[Fergus Henderson]

Hi,

Thanks for the patch.

This one will need to be conditional on both the tests and of course
the underlying gssapi functionality patch.

Here's some comments...

On 3/5/09, Ian....@cern.ch <Ian....@cern.ch> wrote:
>
> + case 'b': {
> + if (opt_whitelist_enabled) {
> + rs_log_error("--whitelist already requested.");

The error message when both --whitelist and --blacklist are specified
should be something like "can't specify both --whitelist and
--blacklist" rather than "--whitelist already requested".

> +#ifdef HAVE_GSSAPI
> + case 'w': {
> + if (opt_blacklist_enabled) {
> + rs_log_error("--blacklist already requested.");

likewise here

> --- quilt-distcc-3.1.orig/distcc-3.1/src/exitcode.h
> +++ quilt-distcc-3.1/distcc-3.1/src/exitcode.h
> @@ -67,7 +67,9 @@ enum dcc_exitcode {

> EXIT_FLAG_MISMATCH_ERROR = 122, /**< GSS-API - Requested flags
> don't match those returned/supported*/

> EXIT_ACQUIRE_CREDS_FAILED = 123, /**< GSS-API - Failed to acquire
> principal credentials for Server*/

> EXIT_NO_PEER_AUTH = 124, /**< GSS-API - Peer not providing
> authentication handshake*/

> - EXIT_NO_PRINCIPAL_NAME = 125 /**< GSS-API - No principal name
> specified for Server*/
> + EXIT_NO_PRINCIPAL_NAME = 125, /**< GSS-API - No principal name
> specified for Server*/
> + EXIT_FAILED_TO_OPEN_LIST_FILE = 126, /**< GSS-API - Failed to open
> black/white list file*/
> + EXIT_LIST_VIOLATION = 127 /**< GSS-API - Connected principal on
> blacklist or not whitelisted*/

Hey, it occurs to me now - and I probably should have thought of it earlier -
that distcc error numbers are pretty much restricted to the range 100..128;
starting from 100 to distinguish them from the return codes of gcc (or
the other C compiler),
and at most 128 because numbers over 128 are for death by signal.
We don't have that many to play with.
So maybe we shouldn't use up so many of them for authentication problems.
Maybe it would be sufficient to have just one (or a small number) of
exit codes for authentication failure,
and require the user to look at the error message from distcc and/or
examine the distccd.log file for the full error message?

Otherwise, this patch looks good.

Cheers,
Fergus.
--
Fergus Henderson <fer...@google.com>