On Fri, Dec 22, 2017 at 10:07:13AM +0000, Frediano Ziglio wrote: > Instead of having half state in RedSASL and half in RedSASLAuth > move everything in RedSASLAuth. This also reduces memory usage > when we are using SASL but we finish the authentication step. > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > --- > server/red-stream.c | 138 +++++++++++++++++++++++++++------------------------- > 1 file changed, 72 insertions(+), 66 deletions(-) > > diff --git a/server/red-stream.c b/server/red-stream.c > index 0e372743a..898253c2e 100644 > --- a/server/red-stream.c > +++ b/server/red-stream.c > @@ -67,13 +67,6 @@ typedef struct RedSASL { > unsigned int encodedOffset; > > SpiceBuffer inbuffer; > - > - char *mechlist; > - char *mechname; > - > - /* temporary data during authentication */ > - unsigned int len; > - char *data; > } RedSASL; > #endif > > @@ -351,13 +344,8 @@ void red_stream_free(RedStream *s) > #if HAVE_SASL > if (s->priv->sasl.conn) { > s->priv->sasl.runSSF = s->priv->sasl.wantSSF = 0; > - s->priv->sasl.len = 0; > s->priv->sasl.encodedLength = s->priv->sasl.encodedOffset = 0; > s->priv->sasl.encoded = NULL; > - g_free(s->priv->sasl.mechlist); > - g_free(s->priv->sasl.mechname); > - s->priv->sasl.mechlist = NULL; > - g_free(s->priv->sasl.data); > sasl_dispose(&s->priv->sasl.conn); > s->priv->sasl.conn = NULL; > } > @@ -735,17 +723,37 @@ static int auth_sasl_check_ssf(RedSASL *sasl, int *runSSF) > > typedef struct RedSASLAuth { > RedStream *stream; > + // list of mech allowed, allocated and freed by SASL mech->mechanisms? > + char *mechlist; > + // mech received > + char *mechname; > + uint32_t len; > + char *data; > + // callback to call if success > RedSaslResult result_cb; > void *result_opaque; > + // saved Async callback, we need to call if failed as > + // we need to chain it in order to use a different opaque data > AsyncReadError saved_error; These 2 comments could have been added in the commit introducing RedSASLAuth. > } RedSASLAuth; > > +static void red_sasl_async_deinit(RedSASLAuth *opaque) > +{ > + g_free(opaque->data); > + opaque->data = NULL; > + g_free(opaque->mechname); > + opaque->mechname = NULL; > + g_free(opaque->mechlist); > + opaque->mechlist = NULL; > + opaque->stream->priv->async_read.error = opaque->saved_error; Not using red_stream_set_async_error_handler() here? (but this also applies to "Handle SASL initialisation mainly in red-stream.c" > +} > + > // handle SASL termination, either success or error > // NOTE: After this function is called usually there should be a > // return or the function should exit > static void red_sasl_async_result(RedSASLAuth *auth, RedSaslError err) > { > - auth->stream->priv->async_read.error = auth->saved_error; > + red_sasl_async_deinit(auth); > auth->result_cb(auth->result_opaque, err); > g_free(auth); > } I would keep the red_stream_set_async_error_handler() call here, and instead have a red_sasl_auth_free() helper: static void red_sasl_async_result(RedSASLAuth *auth, RedSaslError err) { auth->stream->priv->async_read.error = auth->saved_error; auth->result_cb(auth->result_opaque, err); red_sasl_auth_free(auth); } > @@ -753,7 +761,7 @@ static void red_sasl_async_result(RedSASLAuth *auth, RedSaslError err) > static void red_sasl_error(void *opaque, int err) > { > RedSASLAuth *auth = opaque; > - auth->stream->priv->async_read.error = auth->saved_error; > + red_sasl_async_deinit(auth); Same here > if (auth->saved_error) { > auth->saved_error(auth->result_opaque, err); > } > > [...] > > static void red_sasl_handle_auth_mechlen(void *opaque) > { > - RedStream *stream = ((RedSASLAuth *)opaque)->stream; > - RedSASL *sasl = &stream->priv->sasl; > + RedSASLAuth *auth = opaque; > > - sasl->len = GUINT32_FROM_LE(sasl->len); > - if (sasl->len < 1 || sasl->len > SASL_MAX_MECHNAME_LEN) { > - spice_warning("Got bad client mechname len %d", sasl->len); > - return red_sasl_async_result(opaque, RED_SASL_ERROR_INVALID_DATA); > + uint32_t len = GUINT32_FROM_LE(auth->len); You need to update auth->len here: auth->len = GUINT32_FROM_LE(auth->len); uint32_t len = auth->len; I think I would do without the 'len' variable, auth->len is not that long ;) > + if (len < 1 || len > SASL_MAX_MECHNAME_LEN) { > + spice_warning("Got bad client mechname len %d", auth->len); > + return red_sasl_async_result(auth, RED_SASL_ERROR_INVALID_DATA); > } > > - sasl->mechname = g_malloc(sasl->len + 1); > + auth->mechname = g_malloc(len + 1); > > spice_debug("Wait for client mechname"); > - red_stream_async_read(stream, (uint8_t *)sasl->mechname, sasl->len, > - red_sasl_handle_auth_mechname, opaque); > + red_stream_async_read(auth->stream, (uint8_t *)auth->mechname, auth->len, > + red_sasl_handle_auth_mechname, auth); > } > > -bool red_sasl_start_auth(RedStream *stream, RedSaslResult result_cb, void *opaque) > +bool red_sasl_start_auth(RedStream *stream, RedSaslResult result_cb, void *result_opaque) This could go in "Handle SASL initialisation mainly in red-stream.c" > { > const char *mechlist = NULL; > sasl_security_properties_t secprops; > @@ -1047,11 +1054,9 @@ bool red_sasl_start_auth(RedStream *stream, RedSaslResult result_cb, void *opaqu > > spice_debug("Available mechanisms for client: '%s'", mechlist); > > - sasl->mechlist = g_strdup(mechlist); > - > mechlistlen = strlen(mechlist); > if (!red_stream_write_u32(stream, mechlistlen) > - || !red_stream_write_all(stream, sasl->mechlist, mechlistlen)) { > + || !red_stream_write_all(stream, mechlist, mechlistlen)) { > spice_warning("SASL mechanisms write error"); > goto error; > } > @@ -1059,12 +1064,13 @@ bool red_sasl_start_auth(RedStream *stream, RedSaslResult result_cb, void *opaqu > auth = g_new0(RedSASLAuth, 1); > auth->stream = stream; > auth->result_cb = result_cb; > - auth->result_opaque = opaque; > + auth->result_opaque = result_opaque; > auth->saved_error = stream->priv->async_read.error; > - stream->priv->async_read.error = red_sasl_error; > + red_stream_set_async_error_handler(stream, red_sasl_error); Same for this change Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel