[MPlayer-dev-eng] CONF_TYPE_STRING issue

[Ingo Brückl]

Reimar Döffinger wrote on Wed, 4 May 2011 22:49:44 +0200:

> On Wed, May 04, 2011 at 02:51:28PM +0200, Ingo Brückl wrote:
>> Reimar Döffinger wrote on Mon, 2 May 2011 19:16:04 +0200:
>> > The slot data is there to allow "pushing" and "poping" configuration.
>>
>> And the program-accessible variable is "set" from such slot data (strdup'ed
>> in case of char *variable) which leads to the leak.
>>
>> When the slot data and the slot itself (one or multiple slots) have been
>> freed, the allocated data for the program-accessible variable (from the last
>> "set slot to variable") is still present and won't get freed. See attached
>> patch proposal (in which I've added a few more preceding lines for better
>> clarification).

> I don't think that's correct. In MPlayer code you might have
> char *option = "value"; to set some default, and your code would
> end up calling free on that "value" pointer if the option was never
> set, which is not correct.

No, this shouldn't be a problem.

Either the option isn't set in any config, then there will be no parsing and
thus no m_config_option_t *config->opts which means that there will be no
freeing and the "free variable, too" code never gets executed. In this case,
the program uses the default.

Or, the option is set, then there will be a slot containing the data and the
variable is set (strdup'ed) from there. Then there will be a freeing and the
variable get the necessary free(), too. In this case, the program uses the
set option (which is a copy of the parsed data).

In any case, the patch should be save. (At least it was during my testings.)

The only problem is (and this is why I raised this topic in the first place),
may you directly set/alter the variable after parsing? Depending on the
situation this may be ok. For example, it is save to perform

  free(option);
  option = strdup("new_value");

after parsing, if you no further have to rely on the slot data (e.g. no
m_config_set() is done later), but if you do a

  free(option);
  option = "new_value";

after parsing and option was in your config, you'll get a segmentation fault.

The question is: Is the latter considered to be decent programming or should
options only altered by m_config/m_option functions?

Ingo