[MPlayer-dev-eng] [PATCH 4/4] String handling audit/cleanup take 2

[Roberto Togni]

On Fri, 2 Mar 2007 17:31:59 -0500
"Nicholas Kain" <njkain at gmail.com> wrote:

> On 3/2/07, Nicholas Kain <njkain at gmail.com> wrote:
> > I'll attempt to thread the messages beneath this one, barring any
> > smtp delays.
> 
> Patch for the realrtsp subdir.

Very quick review, I'll check it better and apply at least parts of it
next week.

> --- stream/realrtsp/real.c.orig	2007-03-02 08:24:00.000000000
> -0500 +++ stream/realrtsp/real.c	2007-03-02 13:55:44.000000000
> -0500 @@ -56,7 +56,8 @@ static const unsigned char xor_table[] =
>      0x63, 0x11, 0x03, 0x71, 0x08, 0x08, 0x70, 0x02,
>      0x10, 0x57, 0x05, 0x18, 0x54, 0x00, 0x00, 0x00 };
>  
> -
> +#define CHALLENGE_SIZE 64
> +#define CHECKSUM_SIZE 34
>  #define BUF_SIZE 4096
>  
>  #ifdef LOG
> @@ -87,7 +88,7 @@ static void calc_response_string (char *
>    char zres[16];
>    int  i;
>        
> -  av_md5_sum(zres, challenge, 64);
> +  av_md5_sum(zres, challenge, CHALLENGE_SIZE);
>   
>    /* convert zres to ascii string */
>    for (i=0; i<16; i++ ) {
> @@ -109,11 +110,11 @@ static void real_calc_response_and_check
>    char  buf[128];
>  
>    /* initialize return values */
> -  memset(response, 0, 64);
> -  memset(chksum, 0, 34);
> +  memset(response, 0, CHALLENGE_SIZE);
> +  memset(chksum, 0, CHECKSUM_SIZE);

This is ok, but unrelated to the string fixes. I'll apply it.

>  
>    /* initialize buffer */
> -  memset(buf, 0, 128);
> +  memset(buf, 0, sizeof(buf));

Ok

>    ptr=buf;
>    AV_WB32(ptr, 0xa1e9149d);
>    ptr+=4;
> @@ -148,7 +149,7 @@ static void real_calc_response_and_check
>  
>    /* add tail */
>    resp_len = strlen (response);
> -  strcpy (&response[resp_len], "01d0a8e3");
> +  strlcpy (response + resp_len, "01d0a8e3", CHALLENGE_SIZE -
> resp_len); 

from a quick look resp_len is known, so there is no risk of overflow.

>    /* calculate checksum */
>    for (i=0; i<resp_len/4; i++)
> @@ -275,8 +276,9 @@ static rmff_header_t *real_parse_sdp(cha
>  #ifdef LOG
>        printf("asmrp rule match: %u for stream %u\n", rulematches[j],
> desc->stream[i]->stream_id); #endif
> -      sprintf(b,"stream=%u;rule=%u,", desc->stream[i]->stream_id,
> rulematches[j]);
> -      *stream_rules = xbuffer_strcat(*stream_rules, b);
> +      snprintf(b, sizeof(b), "stream=%u;rule=%u,",
> desc->stream[i]->stream_id, rulematches[j]);
> +      *stream_rules = xbuffer_ensure_size(*stream_rules,
> strlen(*stream_rules)+strlen(b)+1);
> +      strlcat(*stream_rules, b, strlen(*stream_rules)+strlen(b)+1);
>      }

Disagree, why remove xbuffer_strcat() ?

>  
>      if (!desc->stream[i]->mlti_data) {
> @@ -443,8 +445,8 @@ rmff_header_t *real_setup_and_get_header
>    char *session_id=NULL;
>    rmff_header_t *h;
>    char *challenge1;
> -  char challenge2[64];
> -  char checksum[34];
> +  char challenge2[CHALLENGE_SIZE];
> +  char checksum[CHECKSUM_SIZE];
>    char *subscribe;
>    char *buf = xbuffer_init(256);
>    char *mrl=rtsp_get_mrl(rtsp_session);
> @@ -466,7 +468,7 @@ rmff_header_t *real_setup_and_get_header
>    /* request stream description */
>  rtsp_send_describe:
>    rtsp_schedule_field(rtsp_session, "Accept: application/sdp");
> -  sprintf(buf, "Bandwidth: %u", bandwidth);
> +  snprintf(buf, 256, "Bandwidth: %u", bandwidth);

Buffer size is 256, this is safe.

>    rtsp_schedule_field(rtsp_session, buf);
>    rtsp_schedule_field(rtsp_session, "GUID:
> 00000000-0000-0000-0000-000000000000");
> rtsp_schedule_field(rtsp_session, "RegionData: 0"); @@ -501,9 +503,9
> @@ rtsp_send_describe: }
>      authlen = strlen(username) + (password ? strlen(password) : 0) +
> 2; authstr = malloc(authlen);
> -    sprintf(authstr, "%s:%s", username, password ? password : "");
> +    snprintf(authstr, authlen, "%s:%s", username, password ?
> password : ""); authfield = malloc(authlen*2+22);
> -    strcpy(authfield, "Authorization: Basic ");
> +    strlcpy(authfield, "Authorization: Basic ", authlen*2+22);

Both for snprintf and strlcpy: the buffer was created in the line above
with the correct size, i see no need to put length checks when writing
into it.

>      b64_authlen = base64_encode(authstr, authlen, authfield+21,
> authlen*2); free(authstr);
>      if (b64_authlen < 0) {
> @@ -564,7 +566,7 @@ autherr:
>  
>    /* parse sdp (sdpplin) and create a header and a subscribe string
> */ subscribe = xbuffer_init(256);
> -  strcpy(subscribe, "Subscribe: ");
> +  strlcpy(subscribe, "Subscribe: ", 256);

Same here, buffer is just created with a size big enough for the string.

>    h=real_parse_sdp(description, &subscribe, bandwidth);
>    if (!h) {
>      subscribe = xbuffer_free(subscribe);

[...]

> --- stream/realrtsp/xbuffer.c.orig	2007-03-02
> 09:21:30.000000000 -0500 +++ stream/realrtsp/xbuffer.c
> 2007-03-02 09:22:22.000000000 -0500 @@ -86,17 +86,3 @@ void
> *xbuffer_ensure_size(void *buf, int return buf;
>  }
>  
> -
> -
> -void *xbuffer_strcat(void *buf, char *data) {

Why?

[..]

Ciao,
 Roberto