[17274] in Kerberos_V5_Development
Re: [PATCH 3/4] Use gssalloc_malloc()/gssalloc_free()
daemon@ATHENA.MIT.EDU (Kevin Wasserman)
Thu Oct 6 15:39:49 2011
Message-ID: <SNT101-DS1880D6EAC56FF34BC8B06DB5F90@phx.gbl>
From: "Kevin Wasserman" <krwasserman@hotmail.com>
To: "Greg Hudson" <ghudson@mit.edu>,
"Sam Hartman" <hartmans@painless-security.com>
In-Reply-To: <1317928128.1548.26.camel@t410>
Date: Thu, 6 Oct 2011 15:39:46 -0400
MIME-Version: 1.0
Cc: Kevin Wasserman <kevin.wasserman@painless-security.com>,
"krbdev@mit.edu" <krbdev@mit.edu>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: krbdev-bounces@mit.edu
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@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@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev
_______________________________________________
krbdev mailing list krbdev@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev
