Commit de451856 authored by Matt Caswell's avatar Matt Caswell
Browse files

Address WPACKET review comments 



A few style tweaks here and there. The main change is that curr and
packet_len are now offsets into the buffer to account for the fact that
the pointers can change if the buffer grows. Also dropped support for the
WPACKET_set_packet_len() function. I thought that was going to be needed
but so far it hasn't been. It doesn't really work any more due to the
offsets change.

Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
parent 6ae4f5e0

ssl/packet.c

+27 −53
Original line number Diff line number Diff line
@@ -25,23 +25,14 @@ int WPACKET_allocate_bytes(WPACKET *pkt, size_t len, unsigned char **allocbytes) 
        if (pkt->buf->length > SIZE_MAX / 2) {

            newlen = SIZE_MAX;

        } else {

            if (pkt->buf->length == 0)

                newlen = DEFAULT_BUF_SIZE;

            else

                newlen = pkt->buf->length * 2;

            newlen = (pkt->buf->length == 0) ? DEFAULT_BUF_SIZE

                                             : pkt->buf->length * 2;

        }

        if (BUF_MEM_grow(pkt->buf, newlen) == 0)

            return 0;

        if (pkt->curr == NULL) {

            /*

             * Can happen if initialised with a BUF_MEM that hasn't been

             * pre-allocated.

             */

            pkt->curr = (unsigned char *)pkt->buf->data;

        }

    }

    pkt->written += len;

    *allocbytes = pkt->curr;

    *allocbytes = (unsigned char *)pkt->buf->data + pkt->curr;

    pkt->curr += len;



    return 1;
@@ -57,12 +48,14 @@ static size_t maxmaxsize(size_t lenbytes) 


int WPACKET_init_len(WPACKET *pkt, BUF_MEM *buf, size_t lenbytes)

{

    unsigned char *lenchars;



    /* Sanity check */

    if (buf == NULL)

        return 0;



    pkt->buf = buf;

    pkt->curr = (unsigned char *)buf->data;

    pkt->curr = 0;

    pkt->written = 0;

    pkt->maxsize = maxmaxsize(lenbytes);
@@ -76,11 +69,12 @@ int WPACKET_init_len(WPACKET *pkt, BUF_MEM *buf, size_t lenbytes) 
    pkt->subs->pwritten = lenbytes;

    pkt->subs->lenbytes = lenbytes;



    if (!WPACKET_allocate_bytes(pkt, lenbytes, &(pkt->subs->packet_len))) {

    if (!WPACKET_allocate_bytes(pkt, lenbytes, &lenchars)) {

        OPENSSL_free(pkt->subs);

        pkt->subs = NULL;

        return 0;

    }

    pkt->subs->packet_len = lenchars - (unsigned char *)pkt->buf->data;



    return 1;

}
@@ -90,25 +84,6 @@ int WPACKET_init(WPACKET *pkt, BUF_MEM *buf) 
    return WPACKET_init_len(pkt, buf, 0);

}



int WPACKET_set_packet_len(WPACKET *pkt, unsigned char *packet_len,

                           size_t lenbytes)

{

    size_t maxmax;



    /* We only allow this to be set once */

    if (pkt->subs == NULL || pkt->subs->lenbytes != 0)

        return 0;



    pkt->subs->lenbytes = lenbytes;

    pkt->subs->packet_len = packet_len;



    maxmax = maxmaxsize(lenbytes);

    if (pkt->maxsize > maxmax)

        pkt->maxsize = maxmax;



    return 1;

}



int WPACKET_set_flags(WPACKET *pkt, unsigned int flags)

{

    if (pkt->subs == NULL)
@@ -126,16 +101,15 @@ int WPACKET_set_flags(WPACKET *pkt, unsigned int flags) 
 */

static int wpacket_intern_close(WPACKET *pkt)

{

    size_t packlen;

    WPACKET_SUB *sub = pkt->subs;

    size_t packlen = pkt->written - sub->pwritten;



    packlen = pkt->written - sub->pwritten;

    if (packlen == 0

            && sub->flags & OPENSSL_WPACKET_FLAGS_NON_ZERO_LENGTH)

            && sub->flags & WPACKET_FLAGS_NON_ZERO_LENGTH)

        return 0;



    if (packlen == 0

            && sub->flags & OPENSSL_WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH) {

            && sub->flags & WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH) {

        /* Deallocate any bytes allocated for the length of the WPACKET */

        if ((pkt->curr - sub->lenbytes) == sub->packet_len) {

            pkt->written -= sub->lenbytes;
@@ -143,17 +117,16 @@ static int wpacket_intern_close(WPACKET *pkt) 
        }



        /* Don't write out the packet length */

        sub->packet_len = NULL;

        sub->packet_len = 0;

        sub->lenbytes = 0;

    }



    /* Write out the WPACKET length if needed */

    if (sub->packet_len != NULL) {

    if (sub->lenbytes > 0) {

        size_t lenbytes;



        lenbytes = sub->lenbytes;



        for (; lenbytes > 0; lenbytes--) {

            sub->packet_len[lenbytes - 1]

        for (lenbytes = sub->lenbytes; lenbytes > 0; lenbytes--) {

            pkt->buf->data[sub->packet_len + lenbytes - 1]

                = (unsigned char)(packlen & 0xff);

            packlen >>= 8;

        }
@@ -187,17 +160,18 @@ int WPACKET_finish(WPACKET *pkt) 
        return 0;



    ret = wpacket_intern_close(pkt);



    if (ret) {

        OPENSSL_free(pkt->subs);

        pkt->subs = NULL;

    }



    return ret;

}



int WPACKET_start_sub_packet_len(WPACKET *pkt, size_t lenbytes)

{

    WPACKET_SUB *sub;

    unsigned char *lenchars;



    if (pkt->subs == NULL)

        return 0;
@@ -212,13 +186,13 @@ int WPACKET_start_sub_packet_len(WPACKET *pkt, size_t lenbytes) 
    sub->lenbytes = lenbytes;



    if (lenbytes == 0) {

        sub->packet_len = NULL;

        sub->packet_len = 0;

        return 1;

    }



    if (!WPACKET_allocate_bytes(pkt, lenbytes, &sub->packet_len)) {

    if (!WPACKET_allocate_bytes(pkt, lenbytes, &lenchars))

        return 0;

    }

    sub->packet_len = lenchars - (unsigned char *)pkt->buf->data;



    return 1;

}
@@ -228,16 +202,15 @@ int WPACKET_start_sub_packet(WPACKET *pkt) 
    return WPACKET_start_sub_packet_len(pkt, 0);

}



int WPACKET_put_bytes(WPACKET *pkt, unsigned int val, size_t bytes)

int WPACKET_put_bytes(WPACKET *pkt, unsigned int val, size_t size)

{

    unsigned char *data;



    if (bytes > sizeof(unsigned int)

            || !WPACKET_allocate_bytes(pkt, bytes, &data))

    if (size > sizeof(unsigned int)

            || !WPACKET_allocate_bytes(pkt, size, &data))

        return 0;



    data += bytes - 1;

    for (; bytes > 0; bytes--) {

    for (data += size - 1; size > 0; size--) {

        *data = (unsigned char)(val & 0xff);

        data--;

        val >>= 8;
@@ -259,7 +232,8 @@ int WPACKET_set_max_size(WPACKET *pkt, size_t maxsize) 
        return 0;



    /* Find the WPACKET_SUB for the top level */

    for (sub = pkt->subs; sub->parent != NULL; sub = sub->parent);

    for (sub = pkt->subs; sub->parent != NULL; sub = sub->parent)

        continue;



    lenbytes = sub->lenbytes;

    if (lenbytes == 0)

ssl/packet_locl.h

+25 −20
Original line number Diff line number Diff line
@@ -557,12 +557,12 @@ struct wpacket_sub { 
    WPACKET_SUB *parent;



    /*

     * Pointer to where the length of this WPACKET goes (or NULL if we don't

     * write the length)

     * Offset into the buffer where the length of this WPACKET goes. We use an

     * offset in case the buffer grows and gets reallocated.

     */

    unsigned char *packet_len;

    size_t packet_len;



    /* Number of bytes in the packet_len */

    /* Number of bytes in the packet_len or 0 if we don't write the length */

    size_t lenbytes;



    /* Number of bytes written to the buf prior to this packet starting */
@@ -577,8 +577,11 @@ struct wpacket_st { 
    /* The buffer where we store the output data */

    BUF_MEM *buf;



    /* Pointer to where we are currently writing */

    unsigned char *curr;

    /*

     * Offset into the buffer where we are currently writing. We use an offset

     * in case the buffer grows and gets reallocated.

     */

    size_t curr;



    /* Number of bytes written so far */

    size_t written;
@@ -593,16 +596,16 @@ struct wpacket_st { 
/* Flags */



/* Default */

#define OPENSSL_WPACKET_FLAGS_NONE                      0

#define WPACKET_FLAGS_NONE                      0



/* Error on WPACKET_close() if no data written to the WPACKET */

#define OPENSSL_WPACKET_FLAGS_NON_ZERO_LENGTH           1

#define WPACKET_FLAGS_NON_ZERO_LENGTH           1



/*

 * Abandon all changes on WPACKET_close() if no data written to the WPACKET,

 * i.e. this does not write out a zero packet length

 */

#define OPENSSL_WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH    2

#define WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH    2





/*
@@ -624,17 +627,6 @@ int WPACKET_init(WPACKET *pkt, BUF_MEM *buf); 
 */

int WPACKET_set_flags(WPACKET *pkt, unsigned int flags);



/*

 * Set the WPACKET length, and the location for where we should write that

 * length. Normally this will be at the start of the WPACKET, and therefore

 * the WPACKET would have been initialised via WPACKET_init_len(). However there

 * is the possibility that the length needs to be written to some other location

 * other than the start of the WPACKET. In that case init via WPACKET_init() and

 * then set the location for the length using this function.

 */

int WPACKET_set_packet_len(WPACKET *pkt, unsigned char *packet_len,

                           size_t lenbytes);



/*

 * Closes the most recent sub-packet. It also writes out the length of the

 * packet to the required location (normally the start of the WPACKET) if
@@ -655,6 +647,19 @@ int WPACKET_finish(WPACKET *pkt); 
 */

int WPACKET_start_sub_packet_len(WPACKET *pkt, size_t lenbytes);



/*

 * Convenience macros for calling WPACKET_start_sub_packet_len with different

 * lengths

 */

#define WPACKET_start_sub_packet_u8(pkt) \

    WPACKET_start_sub_packet_len((pkt), 1)

#define WPACKET_start_sub_packet_u16(pkt) \

    WPACKET_start_sub_packet_len((pkt), 2)

#define WPACKET_start_sub_packet_u24(pkt) \

    WPACKET_start_sub_packet_len((pkt), 3)

#define WPACKET_start_sub_packet_u32(pkt) \

    WPACKET_start_sub_packet_len((pkt), 4)



/*

 * Same as WPACKET_start_sub_packet_len() except no bytes are pre-allocated for

 * the sub-packet length.

ssl/s3_lib.c

+1 −1
Original line number Diff line number Diff line
@@ -2799,7 +2799,7 @@ int ssl3_set_handshake_header2(SSL *s, WPACKET *pkt, int htype) 
{

    /* Set the content type and 3 bytes for the message len */

    if (!WPACKET_put_bytes(pkt, htype, 1)

            || !WPACKET_start_sub_packet_len(pkt, 3))

            || !WPACKET_start_sub_packet_u24(pkt))

        return 0;



    return 1;

ssl/statem/statem_clnt.c

+5 −6
Original line number Diff line number Diff line
@@ -794,7 +794,7 @@ int tls_construct_client_hello(SSL *s) 
    else

        i = s->session->session_id_length;

    if (i > (int)sizeof(s->session->session_id)

            || !WPACKET_start_sub_packet_len(&pkt, 1)

            || !WPACKET_start_sub_packet_u8(&pkt)

            || (i != 0 && !WPACKET_memcpy(&pkt, s->session->session_id, i))

            || !WPACKET_close(&pkt)) {

        SSLerr(SSL_F_TLS_CONSTRUCT_CLIENT_HELLO, ERR_R_INTERNAL_ERROR);
@@ -812,7 +812,7 @@ int tls_construct_client_hello(SSL *s) 
    }



    /* Ciphers supported */

    if (!WPACKET_start_sub_packet_len(&pkt, 2)) {

    if (!WPACKET_start_sub_packet_u16(&pkt)) {

        SSLerr(SSL_F_TLS_CONSTRUCT_CLIENT_HELLO, ERR_R_INTERNAL_ERROR);

        goto err;

    }
@@ -825,7 +825,7 @@ int tls_construct_client_hello(SSL *s) 
    }



    /* COMPRESSION */

    if (!WPACKET_start_sub_packet_len(&pkt, 1)) {

    if (!WPACKET_start_sub_packet_u8(&pkt)) {

        SSLerr(SSL_F_TLS_CONSTRUCT_CLIENT_HELLO, ERR_R_INTERNAL_ERROR);

        goto err;

    }
@@ -852,13 +852,12 @@ int tls_construct_client_hello(SSL *s) 
        SSLerr(SSL_F_TLS_CONSTRUCT_CLIENT_HELLO, SSL_R_CLIENTHELLO_TLSEXT);

        goto err;

    }

    if (!WPACKET_start_sub_packet_len(&pkt, 2)

    if (!WPACKET_start_sub_packet_u16(&pkt)

               /*

                * If extensions are of zero length then we don't even add the

                * extensions length bytes

                */

            || !WPACKET_set_flags(&pkt,

                                  OPENSSL_WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH)

            || !WPACKET_set_flags(&pkt, WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH)

            || !ssl_add_clienthello_tlsext(s, &pkt, &al)

            || !WPACKET_close(&pkt)) {

        ssl3_send_alert(s, SSL3_AL_FATAL, al);

ssl/statem/statem_dtls.c

+1 −0
Original line number Diff line number Diff line
@@ -1200,6 +1200,7 @@ void dtls1_get_message_header(unsigned char *data, struct hm_header_st *msg_hdr) 
int dtls1_set_handshake_header2(SSL *s, WPACKET *pkt, int htype)

{

    unsigned char *header;



    dtls1_set_message_header(s, htype, 0, 0, 0);



    /*