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

[Rich Felker]

On Tue, Mar 06, 2007 at 12:21:48PM +0100, Reimar Döffinger wrote:
> Hello,
> On Fri, Mar 02, 2007 at 05:30:09PM -0500, Nicholas Kain wrote:
> > @@ -690,8 +690,7 @@ static int asf_http_parse_response(asf_h
> >  				  mp_msg(MSGT_NETWORK,MSGL_WARN,MSGTR_MPDEMUX_ASF_ASFHTTPParseWarnCuttedPragma,pragma,len,sizeof(features) - 1);
> >  				  len = sizeof(features) - 1;
> >  				}
> > -				strncpy( features, pragma, len );
> > -				features[len]='\0';
> > +				strlcpy( features, pragma, len );
> 
> wrong.
> 
> >      dst = malloc(length + 1);
> > -    strncpy(dst, src, length);
> > -    dst[length] = 0;
> > +    strlcpy(dst, src, length);
> 
> wrong.

Just replace length with length+1 (provided you know it doesn't
overflow which you hopefully already checked before malloc...)

> > @@ -389,7 +388,7 @@ http_response_parse( HTTP_header_t *http
> >  		mp_msg(MSGT_NETWORK,MSGL_FATAL,"Memory allocation failed\n");
> >  		return -1;
> >  	}
> > -	strncpy( http_hdr->reason_phrase, hdr_ptr, len );
> > +	strlcpy( http_hdr->reason_phrase, hdr_ptr, len );
> >  	if( http_hdr->reason_phrase[len-1]=='\r' ) {
> 
> wrong, and this condition will always be false then

Indeed, but this doesn't make using strncpy correct. strncpy is for
fixed-field-size databases and should _never_ be used except for that
purpose..

> > @@ -453,8 +453,7 @@ cddb_parse_matches_list(HTTP_header_t *h
> >  		} else {
> >  			len = ptr2-ptr+1;
> >  		}
> > -		strncpy(album_title, ptr, len);
> > -		album_title[len-2]='\0';
> > +		strlcpy(album_title, ptr, len);
> >  	}
> >  	mp_msg(MSGT_DEMUX, MSGL_STATUS, MSGTR_MPDEMUX_CDDB_ParseOKFoundAlbumTitle, album_title);
> 
> These are not equivalent, but don't ask me which one is wrong...

:)

> > -  snprintf(str,127,"%d.%d.%d.%d",num[0],num[1],num[2],num[3]);
> > +  snprintf(str,sizeof(str),"%d.%d.%d.%d",num[0],num[1],num[2],num[3]);
> 
> Wow, I'd say in that file there was a proponent of useless use of
> snprintf at work.
> I must say, having an integer becoming >= 10^30 is really likely... We
> might really risk an overflow once ints become 128bit, with the first
> 1024-bit CPUs...

Useless? Why? It does not cost anything and provides documentation
that the code is safe. Without such documentation, auditing is a
nightmare.

> > --- stream/stream_rtsp.c.orig	2007-03-02 11:24:10.000000000 -0500
> > +++ stream/stream_rtsp.c	2007-03-02 13:39:52.000000000 -0500
> > @@ -61,6 +61,7 @@ rtsp_streaming_start (stream_t *stream)
> >    char *file;
> >    int port;
> >    int redirected, temp;
> > +  size_t len;
> >  
> >    if (!stream)
> >      return -1;
> > @@ -87,10 +88,10 @@ rtsp_streaming_start (stream_t *stream)
> >      if (file[0] == '/')
> >        file++;
> >  
> > -    mrl = malloc (strlen (stream->streaming_ctrl->url->hostname)
> > -                  + strlen (file) + 16);
> > +    len = strlen (stream->streaming_ctrl->url->hostname) + strlen (file) + 16;
> > +    mrl = malloc (len);
> >      
> > -    sprintf (mrl, "rtsp://%s:%i/%s",
> > +    snprintf (mrl, sizeof(len), "rtsp://%s:%i/%s",
> >               stream->streaming_ctrl->url->hostname, port, file);
> 
> wtf?

Indeed, sizeof is wrong there.

> > --- stream/stream_smb.c.orig	2007-03-02 07:35:09.000000000 -0500
...
> Nice code duplication at work here... And did someone think here the
> string "LAN" would become higher quality by copying it around a few
> times? We seriously need proper maintainers for some of that code...

:)

Rich