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

[Reimar Döffinger]

Hello,
On Fri, Mar 02, 2007 at 05:28:54PM -0500, Nicholas Kain wrote:
> --- asxparser.c.orig	2007-03-02 02:21:30.000000000 -0500
> +++ asxparser.c	2007-03-02 16:08:57.000000000 -0500
> @@ -118,13 +118,16 @@ asx_warning_attrib_invalid(ASX_Parser_t*
>      len += strlen(ptr[0]);
>      len += ((ptr[1] == NULL) ? 4 : 2);
>    }
> -  str = vals = malloc(len);
> -  vals += sprintf(vals,"%s",valid_vals[0]);
> +  str = malloc(len);
> +  strlcpy(str, valid_vals[0],len);
>    for(ptr = valid_vals + 1 ; ptr[0] != NULL ; ptr++) {
> -    if(ptr[1] == NULL)
> -      vals += sprintf(vals," or %s",ptr[0]);
> -    else
> -      vals += sprintf(vals,", %s",ptr[0]);
> +    if(ptr[1] == NULL) {
> +      strlcat(str, " or ", len);
> +      strlcat(str, ptr[0], len);
> +    } else {
> +      strlcat(str, ", ", len);
> +      strlcat(str, ptr[0], len);
> +    }

Now, this IMO is an example of a good change. But not of the best
possible solution because
1) output of "remote" content in the end should preferably be filtered,
I think there are some console escape sequences that can do some really
non-nice things

>    mp_msg(MSGT_PLAYTREE,MSGL_ERR,"at line %d : attribute %s of element %s is invalid (%s). Valid values are %s",
>  	      parser->line,attrib,elem,val,str);

2) We could just avoid all this string handling problems by splitting
this mp_msg and call mp_msg instead of sprintf in the loop.

Greetings,
Reimar Döffinger
> --- command.c.orig	2007-03-02 05:32:44.000000000 -0500
> +++ command.c	2007-03-02 13:27:51.000000000 -0500
> @@ -290,11 +290,11 @@ static int mp_property_length(m_option_t
>  	    s -= m * 60;
>  	    *(char **) arg = malloc(20);
>  	    if (h > 0)
> -		sprintf(*(char **) arg, "%d:%02d:%02d", h, m, s);
> +		snprintf(*(char **) arg, 20, "%d:%02d:%02d", h, m, s);
>  	    else if (m > 0)
> -		sprintf(*(char **) arg, "%d:%02d", m, s);
> +		snprintf(*(char **) arg, 20, "%d:%02d", m, s);
>  	    else
> -		sprintf(*(char **) arg, "%d", s);
> +		snprintf(*(char **) arg, 20, "%d", s);

Probably a good example for why we should consider asprintf + our own
implementation of it in osdep for systems where it's not available.
Or just doing malloc(100) and not have to think about it further.

> @@ -973,7 +973,6 @@ static int mp_property_sub(m_option_t * 
>  	if (!arg)
>  	    return M_PROPERTY_ERROR;
>  	*(char **) arg = malloc(64);
> -	(*(char **) arg)[63] = 0;
>  	sub_name = 0;
>  	if (subdata)
>  	    sub_name = subdata->filename;
> @@ -987,7 +986,7 @@ static int mp_property_sub(m_option_t * 
>  	    if ((tmp2 = strrchr(tmp, '/')))
>  		tmp = tmp2 + 1;
>  
> -	    snprintf(*(char **) arg, 63, "(%d) %s%s",
> +	    snprintf(*(char **) arg, 64, "(%d) %s%s",
>  		     mpctx->set_of_sub_pos + 1,
>  		     strlen(tmp) < 20 ? "" : "...",
>  		     strlen(tmp) < 20 ? tmp : tmp + strlen(tmp) - 19);
> @@ -998,7 +997,7 @@ static int mp_property_sub(m_option_t * 
>  	    if (vo_spudec && dvdsub_id >= 0) {
>  		unsigned char lang[3];
>  		if (dvdnav_lang_from_sid(mpctx->stream, dvdsub_id, lang)) {
> -		    snprintf(*(char **) arg, 63, "(%d) %s", dvdsub_id, lang);
> +		    snprintf(*(char **) arg, 64, "(%d) %s", dvdsub_id, lang);
>  		    return M_PROPERTY_OK;
>  		}
>  	    }
> @@ -1008,7 +1007,7 @@ static int mp_property_sub(m_option_t * 
>  	if (mpctx->demuxer->type == DEMUXER_TYPE_MATROSKA && dvdsub_id >= 0) {
>  	    char lang[40] = MSGTR_Unknown;
>  	    demux_mkv_get_sub_lang(mpctx->demuxer, dvdsub_id, lang, 9);
> -	    snprintf(*(char **) arg, 63, "(%d) %s", dvdsub_id, lang);
> +	    snprintf(*(char **) arg, 64, "(%d) %s", dvdsub_id, lang);
>  	    return M_PROPERTY_OK;
>  	}
>  #ifdef HAVE_OGGVORBIS
> @@ -1016,14 +1015,14 @@ static int mp_property_sub(m_option_t * 
>  	    char *lang = demux_ogg_sub_lang(mpctx->demuxer, dvdsub_id);
>  	    if (!lang)
>  		lang = MSGTR_Unknown;
> -	    snprintf(*(char **) arg, 63, "(%d) %s", dvdsub_id, lang);
> +	    snprintf(*(char **) arg, 64, "(%d) %s", dvdsub_id, lang);
>  	    return M_PROPERTY_OK;
>  	}
>  #endif
>  	if (vo_vobsub && vobsub_id >= 0) {
>  	    const char *language = MSGTR_Unknown;
>  	    language = vobsub_get_id(vo_vobsub, (unsigned int) vobsub_id);
> -	    snprintf(*(char **) arg, 63, "(%d) %s",
> +	    snprintf(*(char **) arg, 64, "(%d) %s",
>  		     vobsub_id, language ? language : MSGTR_Unknown);
>  	    return M_PROPERTY_OK;
>  	}
> @@ -1035,15 +1034,15 @@ static int mp_property_sub(m_option_t * 
>  	    lang[0] = code >> 8;
>  	    lang[1] = code;
>  	    lang[2] = 0;
> -	    snprintf(*(char **) arg, 63, "(%d) %s", dvdsub_id, lang);
> +	    snprintf(*(char **) arg, 64, "(%d) %s", dvdsub_id, lang);
>  	    return M_PROPERTY_OK;
>  	}
>  #endif
>  	if (dvdsub_id >= 0) {
> -	    snprintf(*(char **) arg, 63, "(%d) %s", dvdsub_id, MSGTR_Unknown);
> +	    snprintf(*(char **) arg, 64, "(%d) %s", dvdsub_id, MSGTR_Unknown);
>  	    return M_PROPERTY_OK;
>  	}
> -	snprintf(*(char **) arg, 63, MSGTR_Disabled);
> +	snprintf(*(char **) arg, 64, MSGTR_Disabled);
>  	return M_PROPERTY_OK;

Looks fine, code based on confusion of snprintf vs. strncpy behaviour.

I guess I should go about applying those I indicated, any objections to
these?

Greetings,
Reimar Döffinger