Menu
▾
▴
gnuplot-beta
|
From: Juergen W. <wie...@fr...> - 2006-07-13 07:43:23
Attachments:
no-stringvars-2006-07-13.diff
|
Hi,
the attached patch contains the minimal changes to reenable
compiling and linking with --disable-stringvars. I think this
switch should be dropped somewhen.
While in it, the snippet:
v = add_udv_by_name("GPVAL_TERM");
if (v) {
v->udv_undef = FALSE;
Gstring(&v->udv_value,(char*)term->name); /* this can be pointer */
}
looks dangerous to me. The string a variable points to may be freed
if the variable is reset. Even if it might be save in this special
case (I don't know), it is bad style in my opinion. Would an
additional function Gstring_copy() be useful, which could be called
in places like this?
Juergen
|
|
From: Juergen W. <wie...@fr...> - 2006-07-13 07:59:57
|
> the attached patch contains the minimal changes to reenable > compiling and linking with --disable-stringvars. Had another look into it. This would also disable the 'defined' function within gnuplot. AFAICS, the solution is to give it any pointer, including NULL, instead of f_exists. But I'm not really comfortable with the defined-code. Juergen |
|
From: Ethan A M. <merritt@u.washington.edu> - 2006-07-13 15:14:13
|
On Thursday 13 July 2006 12:43 am, Juergen Wieferink wrote:
>
> While in it, the snippet:
>
> v = add_udv_by_name("GPVAL_TERM");
> if (v) {
> v->udv_undef = FALSE;
> Gstring(&v->udv_value,(char*)term->name); /* this can be pointer */
> }
>
> looks dangerous to me. The string a variable points to may be freed
> if the variable is reset.
I take it that you are worried some other bit of code might call
gpfree_string(add_udv_by_name("GPVAL_TERM")), thus triggering
an error when it tries to free the static allocation of term->name?
The GPVAL_* variables cannot be over-written by the user,
and this one bit of code is the only place that gnuplot itself
sets this variable. So it is safe.
> Would an additional function Gstring_copy() be useful, which could be called
> in places like this?
I don't think that is necessary. You can already do this easily:
Gstring(&v->udv_value, gp_strdup(term->name));
Then you would have to modify the snippet of code above to deal with the
opposite problem, that unless the previous name is explicitly freed there
will be a memory leak.
--
Ethan A Merritt
Biomolecular Structure Center
University of Washington, Seattle 98195-7742
|
|
From: Juergen W. <wie...@fr...> - 2006-07-13 18:27:41
|
On Thursday 13 July 2006 17:14 Ethan A Merritt wrote:
> On Thursday 13 July 2006 12:43 am, Juergen Wieferink wrote:
> > While in it, the snippet:
> >
> > v = add_udv_by_name("GPVAL_TERM");
> > if (v) {
> > v->udv_undef = FALSE;
> > Gstring(&v->udv_value,(char*)term->name); /* this can be pointer */
> > }
> >
> > looks dangerous to me. The string a variable points to may be freed
> > if the variable is reset.
>
> I take it that you are worried some other bit of code might call
> gpfree_string(add_udv_by_name("GPVAL_TERM")), thus triggering
> an error when it tries to free the static allocation of term->name?
>
> The GPVAL_* variables cannot be over-written by the user,
> and this one bit of code is the only place that gnuplot itself
> sets this variable. So it is safe.
I don't like code like that nevertheless. But it's not within my
responsibility.
> > Would an additional function Gstring_copy() be useful, which could be
> > called in places like this?
>
> I don't think that is necessary. You can already do this easily:
>
> Gstring(&v->udv_value, gp_strdup(term->name));
Good point!
> Then you would have to modify the snippet of code above to deal with the
> opposite problem, that unless the previous name is explicitly freed there
> will be a memory leak.
Well, it could be done like the others:
fill_gpval_string("GPVAL_TERM", term->name);
Never mind.
I do see another problem in fill_gpval_string():
fill_gpval_string(char *var, char *value)
{
#ifdef GP_STRING_VARS
struct udvt_entry *v = add_udv_by_name(var);
if (!v)
return;
if (v->udv_undef == FALSE && !strcmp((char*)&v->udv_value, value))
return;
v->udv_undef = FALSE;
gpfree_string(&v->udv_value);
Gstring(&v->udv_value, gp_strdup(value));
/* fprintf(stderr, "now it is: |%s|\n", &v->udv_value.v.string_val); */
#endif
}
When this function is invoked with a new variable, *v is newly
allocated and v->udv_value is not initialized. It may happen to be
(v->udv_value->type == STRING && v->udv_value->string_val != NULL),
in which (quite improbable) case one would trigger a hardly
reproducible segfault.
I'd suggest something like:
Index: eval.c
===================================================================
--- eval.c (Revision 279)
+++ eval.c (Arbeitskopie)
@@ -733,8 +733,10 @@
return;
if (v->udv_undef == FALSE && !strcmp((char*)&v->udv_value, value))
return;
- v->udv_undef = FALSE;
- gpfree_string(&v->udv_value);
+ if (v->udv_undef)
+ v->udv_undef = FALSE;
+ else
+ gpfree_string(&v->udv_value);
Gstring(&v->udv_value, gp_strdup(value));
/* fprintf(stderr, "now it is: |%s|\n", &v->udv_value.v.string_val); */
#endif
Juergen
|
|
From: Ethan M. <merritt@u.washington.edu> - 2006-07-14 00:09:31
|
On Thursday 13 July 2006 11:27 am, Juergen Wieferink wrote:
>
> Well, it could be done like the others:
>
> fill_gpval_string("GPVAL_TERM", term->name);
Fine.
> I'd suggest something like:
>
> Index: eval.c
> ===================================================================
> --- eval.c (Revision 279)
> +++ eval.c (Arbeitskopie)
> @@ -733,8 +733,10 @@
> return;
> if (v->udv_undef == FALSE && !strcmp((char*)&v->udv_value,
> value)) return;
> - v->udv_undef = FALSE;
> - gpfree_string(&v->udv_value);
> + if (v->udv_undef)
> + v->udv_undef = FALSE;
> + else
> + gpfree_string(&v->udv_value);
> Gstring(&v->udv_value, gp_strdup(value));
Agreed. Both now in cvs.
--
Ethan A Merritt
Biomolecular Structure Center
University of Washington, Seattle WA
|
