[PATCH 3/4] Use gssalloc_malloc()/gssalloc_free() withgss_buffer_t.

[Kevin Wasserman]

I agree with your second comment and will clean up the code.
For this patch, I tried to make as minimal changes as possible
to make the code 'correct'.   I am concerned that you say you
get a crash when logfile is set.  In the first block, because msg_buf
is a gss_buffer_desc, not a gss_buffer_t, it should not be
affected by invalidating xmit_buf; the assignment in the previous
statement should copy the whole structure, not just  a pointer.
I set xmit_buf's value to NULL to avoid a potential double-free
later, since msg_buf takes ownership of the memory.
What configuration were you using to get the crash?

Again, though, I agree that this code is pretty badly structured
and I'll clean it up along the lines you suggest.

-----Original Message----- 
From: Greg Hudson
Sent: Thursday, October 06, 2011 3:08 PM
To: Sam Hartman
Cc: Kevin Wasserman ; krbdev at mit.edu
Subject: Re: [PATCH 3/4] Use gssalloc_malloc()/gssalloc_free() 
withgss_buffer_t.

On Thu, 2011-10-06 at 13:25 -0400, Sam Hartman wrote:
> @@ -495,6 +496,7 @@ sign_server(int s, gss_cred_id_t server_creds, int 
> export)
>              }
>          } else {
>              msg_buf = xmit_buf;
> +            xmit_buf.value = 0;

This seems broken.  msg_buf is supposed to contain the plain text of the
message when this block is done.  Instead, this causes msg_buf to be
invalid (leading to a crash at least if logfile is set), and xmit_buf's
memory to be leaked.

> +        if (msg_buf.value) {
> +            if (token_flags & TOKEN_WRAPPED) {
> +                gss_release_buffer(&min_stat, &msg_buf);
> +            }
> +            else {
>                  free(msg_buf.value);
>                  msg_buf.value = 0;
>              }
> +        }

Tracking the allocation status of a buffer using a superficially
unrelated variable is a maintenance trap.  Keep separately owned memory
in separate variables.  That is, make a recv_buf object to hold the
locally allocated received-message buffer and an unwrap_buf object to
possibly hold the unwrapped value.  Then use a pointer to decide which
buffer to use as input for sending.


_______________________________________________
krbdev mailing list             krbdev at mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev