[MPlayer-dev-eng] [PATCH 2/4] String handling audit/cleanup take 2
Reimar Döffinger
Reimar.Doeffinger at stud.uni-karlsruhe.de
Tue Mar 6 12:21:48 CET 2007
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.
> @@ -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
> @@ -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...
> @@ -490,8 +489,7 @@ cddb_query_parse(HTTP_header_t *http_hdr
> } 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);
yummy, more code duplication (not by you, the original code I mean)...
> --- stream/stream_cue.c.orig 2007-03-02 08:00:52.000000000 -0500
> +++ stream/stream_cue.c 2007-03-02 13:43:17.000000000 -0500
> @@ -33,6 +33,7 @@
> #define MODE1_2048 30
> #define MODE2_2336 40
> #define UNKNOWN -1
> +#define CUEBUFMAX 256
PATH_MAX probably is more appropriate but the malloc should be used...
> @@ -156,14 +157,10 @@ static int cue_getTrackinfo(char *Line,
>
>
>
> -/* FIXME: the string operations ( strcpy,strcat ) below depend
> - * on the arrays to have the same size, thus we need to make
> - * sure the sizes are in sync.
> - */
This is wrong. If they are not the same size the code still won't work
with those changes, it just won't write beyond a buffer.
No buffer overflow != correct code.
> bin_filename);
>
> /* now try to find it with the path of the cue file */
> - snprintf(s,sizeof( s ),"%s/%s",bincue_path,bin_filename);
> + snprintf(s,sizeof(s),"%s/%s",bincue_path,bin_filename);
cosmetics
> @@ -221,23 +217,22 @@ static int cue_find_bin (char *firstline
> MSGTR_MPDEMUX_CUEREAD_BinFilenameTested, s);
>
> /* ok try it with path */
> - snprintf(t, sizeof( t ), "%s/%s", bincue_path, s);
> + snprintf(t, sizeof(t), "%s/%s", bincue_path, s);
Here, too.
> - snprintf(t, sizeof( t ), "%s/%s", bincue_path, s);
> + snprintf(t, sizeof(t), "%s/%s", bincue_path, s);
And here.
> @@ -322,7 +317,7 @@ static int cue_read_cue (char *in_cue_fi
> *t = '\0';
> t = s;
> if (*t == '\0')
> - strcpy(t, "/");
> + strlcpy(t, "/", 2);
pointless. Code could probably be simplified though.
> if (response[3] == '-') {
> - strncpy(match,response,3);
> + strncpy(match,response,3); /* safe and intended */
> match[3] = ' ';
> match[4] = '\0';
Hm.
strlcpy(match, response, 5);
match[3] = ' ';
would be no worse though.
Although the 0-termination of match is pointless anyway...
> - 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...
> --- 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?
> --- stream/stream_smb.c.orig 2007-03-02 07:35:09.000000000 -0500
> +++ stream/stream_smb.c 2007-03-02 07:43:53.000000000 -0500
> @@ -35,24 +35,28 @@ static void smb_auth_fn(const char *serv
> char *password, int pwmaxlen)
> {
> char temp[128];
> + size_t len;
>
> - strcpy(temp, "LAN");
> - if (temp[strlen(temp) - 1] == 0x0a)
> - temp[strlen(temp) - 1] = 0x00;
> + strlcpy(temp, "LAN", sizeof(temp));
> + len = strlen(temp) - 1;
> + if (len > 0 && temp[len] == 0x0a)
> + temp[len] = 0x00;
>
> - if (temp[0]) strncpy(workgroup, temp, wgmaxlen - 1);
> + if (temp[0]) strlcpy(workgroup, temp, wgmaxlen);
>
> - strcpy(temp, smb_username);
> - if (temp[strlen(temp) - 1] == 0x0a)
> - temp[strlen(temp) - 1] = 0x00;
> + strlcpy(temp, smb_username, sizeof(temp));
> + len = strlen(temp) - 1;
> + if (len > 0 && temp[len] == 0x0a)
> + temp[len] = 0x00;
>
> - if (temp[0]) strncpy(username, temp, unmaxlen - 1);
> + if (temp[0]) strlcpy(username, temp, unmaxlen);
>
> - strcpy(temp, smb_password);
> - if (temp[strlen(temp) - 1] == 0x0a)
> - temp[strlen(temp) - 1] = 0x00;
> + strlcpy(temp, smb_password, sizeof(temp));
> + len = strlen(temp) - 1;
> + if (len > 0 && temp[len] == 0x0a)
> + temp[len] = 0x00;
>
> - if (temp[0]) strncpy(password, temp, pwmaxlen - 1);
> + if (temp[0]) strlcpy(password, temp, pwmaxlen);
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...
> static int control(stream_t *s, int cmd, void *arg) {
> --- stream/tv.h.orig 2007-03-02 07:34:11.000000000 -0500
> +++ stream/tv.h 2007-03-02 07:34:30.000000000 -0500
> @@ -97,7 +97,6 @@ typedef struct tv_channels_s {
>
> extern tv_channels_t *tv_channel_list;
> extern tv_channels_t *tv_channel_current, *tv_channel_last;
> -extern char *tv_channel_last_real;
Somewhat unrelated (yes I saw the changes below that make it necessary).
> @@ -101,8 +102,7 @@ url_new(const char* url) {
> mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed);
> goto err_out;
> }
> - strncpy(Curl->protocol, escfilename, pos1);
> - Curl->protocol[pos1] = '\0';
> + strlcpy(Curl->protocol, escfilename, pos1);
wrong.
> @@ -123,8 +123,7 @@ url_new(const char* url) {
> mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed);
> goto err_out;
> }
> - strncpy(Curl->username, ptr1, len);
> - Curl->username[len] = '\0';
> + strlcpy(Curl->username, ptr1, len);
wrong.
> @@ -136,8 +135,7 @@ url_new(const char* url) {
> mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed);
> goto err_out;
> }
> - strncpy( Curl->password, ptr3+1, len2);
> - Curl->password[len2]='\0';
> + strlcpy( Curl->password, ptr3+1, len2);
wrong.
> @@ -188,8 +186,7 @@ url_new(const char* url) {
> mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed);
> goto err_out;
> }
> - strncpy(Curl->hostname, ptr1, pos2-pos1);
> - Curl->hostname[pos2-pos1] = '\0';
> + strlcpy(Curl->hostname, ptr1, pos2-pos1);
wrong.
> @@ -212,7 +209,7 @@ url_new(const char* url) {
> mp_msg(MSGT_NETWORK,MSGL_FATAL,MSGTR_MemAllocFailed);
> goto err_out;
> }
> - strcpy(Curl->file, "/");
> + strlcpy(Curl->file, "/", 2);
pointless.
> @@ -322,7 +319,7 @@ url_escape_string(char *outbuf, const ch
> if(tmp && (tmp[1] == '/' || tmp[1] == ':' ||
> tmp[1] == '\0')) {
> i = tmp+1-inbuf;
> - strncpy(outbuf,inbuf,i);
> + strncpy(outbuf,inbuf,i); /* safe and intended */
> outbuf += i;
> tmp = NULL;
Using strlcpy would work fine too though.
But the whole url_escape_string function is a horror and lacks at least
some proper documentation of how to use it safely.
And that tmp = NULL; must be one level further out AFAICT.
Greetings,
Reimar Döffinger
More information about the MPlayer-dev-eng
mailing list