On Mon, 2002-04-15 at 22:32, Not Zed wrote:
> On Mon, 2002-04-15 at 18:11, Russell Steinthal wrote:
> > Fellow hackers:
[snip]
> > I don't know who the appropriate maintainer is for this section of code,
> > so I decided to send it here. If the appropriate person wants to reveal
> > himself, I'm happy to take suggested changes, instructions on whether to
> > commit, where to send this, etc...
>
> evolution-patches I suppose. Its usually useful to create a bug to
> track it and attach it there too.
>
> I can't really comment on the calendar interaction code, but the rest of
> it looks a little questionable on several fronts.
Thanks for the comments...
> For example, if the string contains 2 %d's, the second will access a
> dereferenced alarm, or if it contains none, then the alarm is leaked (if
> i understand the alarm_get/free semantics which appear in other parts of
> that file). Once you start getting user-supplied strings, you have to
> assume the data wont be correct.
Yup, that's a bug. I think it should be sufficient to move the
cal_component_alarm_free (alarm) out of the switch and put it at the end
along with the g_strfreev (which executes exactly once per function
invocation).
> Using the g_strsplit and g_strjoin stuff is pretty messy - its much
> simpler and more robust to just use a g_string with g_string_append_c
> etc, and iterate through the source string char by char. It'll probably
> reduce your code by 10% and save executing >50% of what it does now, and
> be more readable.
What would have been much simpler and cleaner would have been if I could
have found a glib-like substitution function already written. :) I
tried doing a straight character by character iteration, but needed to
constantly grow the string, which looked to be even more sketchy. I
suppose g_string avoids that problem...
> Finally, none of the arguments are escaped when put into the command
> line, which might introduce potential problems - but that sort of
> depends on how the string is specified & used, etc.
True, although since the arguments are already user-specified, there's
no real added security risk. I suppose Evolution could quote the
arguments as they are substituted, which would avoid the need to do -s
"%s" for a multi-word value, but beyond that, I think we'll (I'll) need
to trust the user.
Thanks again for the comments--- I'll probably start a rewrite from
scratch using g_string, quoting, etc., and generalize it for e-util/ in
light of bug #23329 (asking for similar functionality for the mailer).
The substitution code can be abstracted out, and the mailer/calendar can
just fill the substitution mapping with their own values.
-Russell
>
> > -Russell
> >
> > Index: calendar/ChangeLog
> > ===================================================================
> > RCS file: /cvs/gnome/evolution/calendar/ChangeLog,v
> > retrieving revision 1.1356
> > diff -u -r1.1356 ChangeLog
> > --- calendar/ChangeLog 10 Apr 2002 22:00:45 -0000 1.1356
> > +++ calendar/ChangeLog 15 Apr 2002 08:27:34 -0000
> > @@ -1,3 +1,10 @@
> > +2002-04-15 Russell Steinthal <[EMAIL PROTECTED]>
> > +
> > + * gui/alarm-notify/alarm-queue.c (alarm_replace_strings): New
> > + function to support interpolation of summary, description,
> > + appointment start times into arguments for procedure alarms.
> > + Currently called only from procedure_notification ()
> > +
> > 2002-04-10 Dan Winship <[EMAIL PROTECTED]>
> >
> > * gui/gnome-cal.c (gnome_calendar_open): Fix this: Rodrigo's patch
> > Index: calendar/gui/alarm-notify/alarm-queue.c
> > ===================================================================
> > RCS file: /cvs/gnome/evolution/calendar/gui/alarm-notify/alarm-queue.c,v
> > retrieving revision 1.21
> > diff -u -r1.21 alarm-queue.c
> > --- calendar/gui/alarm-notify/alarm-queue.c 14 Jan 2002 01:07:09 -0000 1.21
> > +++ calendar/gui/alarm-notify/alarm-queue.c 15 Apr 2002 08:27:44 -0000
> > @@ -110,7 +110,7 @@
> > static void audio_notification (time_t trigger, CompQueuedAlarms *cqa, gpointer
>alarm_id);
> > static void mail_notification (time_t trigger, CompQueuedAlarms *cqa, gpointer
>alarm_id);
> > static void procedure_notification (time_t trigger, CompQueuedAlarms *cqa,
>gpointer alarm_id);
> > -
> > +static gchar* alarm_replace_strings (gchar *str, time_t trigger, CompQueuedAlarms
>*cqa, gpointer alarm_id);
> >
> >
> > /* Alarm queue engine */
> > @@ -793,6 +793,7 @@
> > icalattach *attach;
> > const char *url;
> > char *cmd;
> > + char *subst_cmd;
> > int result;
> >
> > comp = cqa->alarms->comp;
> > @@ -825,12 +826,16 @@
> > cmd = (char *) url;
> >
> > result = 0;
> > +
> > + subst_cmd = alarm_replace_strings (cmd, trigger, cqa, alarm_id);
> > if (procedure_notification_dialog (cmd, url))
> > - result = gnome_execute_shell (NULL, cmd);
> > + result = gnome_execute_shell (NULL, subst_cmd);
> >
> > if (cmd != (char *) url)
> > g_free (cmd);
> >
> > + g_free (subst_cmd);
> > +
> > icalattach_unref (attach);
> >
> > /* Fall back to display notification if we got an error */
> > @@ -1028,4 +1033,77 @@
> > g_free (ca);
> >
> > g_hash_table_remove (client_alarms_hash, client);
> > +}
> > +
> > +static gchar*
> > +alarm_replace_strings (gchar *str, time_t trigger, CompQueuedAlarms *cqa,
>gpointer alarm_id)
> > +{
> > + CalComponent *comp;
> > + CalComponentAlarm *alarm;
> > + CalComponentText text;
> > + QueuedAlarm *qa;
> > +
> > + gchar **src_tokens, **dest_tokens;
> > + gchar **src, **dest, *ret;
> > + int num_tokens;
> > +
> > + /* Find the alarm structures */
> > + comp = cqa->alarms->comp;
> > + qa = lookup_queued_alarm (cqa, alarm_id);
> > + g_assert (qa != NULL);
> > + alarm = cal_component_get_alarm (comp, qa->instance->auid);
> > + g_assert (alarm != NULL);
> > +
> > + src_tokens = g_strsplit (str, "%", 0);
> > + src = src_tokens;
> > + num_tokens = 0;
> > + while (*(src++)) {
> > + num_tokens++;
> > + }
> > +
> > + dest_tokens = g_new0 (gchar*, num_tokens * 2);
> > +
> > + /* The first token can't contain a replacement character */
> > + src = src_tokens;
> > + dest = dest_tokens;
> > + *(dest++) = g_strdup (*(src++));
> > +
> > + while (*src) {
> > + switch (**src) {
> > + case 's':
> > + cal_component_get_summary (comp, &text);
> > + *(dest++) = g_strdup (text.value ? text.value : _("No
>description available"));
> > + *(dest++) = g_strdup (*src+1);
> > + break;
> > + case 'd':
> > + cal_component_alarm_get_description (alarm, &text);
> > + cal_component_alarm_free (alarm);
> > + *(dest++) = g_strdup (text.value ? text.value : _("No
>description available"));
> > + *(dest++) = g_strdup (*src+1);
> > + break;
> > + case 't':
> > + *(dest++) = g_strdup_printf ("%ld",
>qa->instance->occur_start); /* FIXME */
> > + *(dest++) = g_strdup (*src+1);
> > + break;
> > + case 'T':
> > + *(dest++) = g_strdup ((gchar*) ctime
>(&qa->instance->occur_start)); /* FIXME */
> > + *(dest++) = g_strdup (*src+1);
> > + break;
> > + case '\0':
> > + *(dest++) = g_strdup ("%");
> > + if (*(src+1) && **(src+1) == '\0')
> > + src++;
> > + break;
> > + default:
> > + *(dest++) = g_strdup (*src+1);
> > + break;
> > + }
> > + src++;
> > + }
> > +
> > + dest = NULL;
> > + ret = g_strjoinv ("", dest_tokens);
> > + g_strfreev (dest_tokens);
> > + g_strfreev (src_tokens);
> > + return ret;
> > }
> >
> > --
> > Russell Steinthal Columbia Law School, Class of 2002
> > <[EMAIL PROTECTED]> Columbia College, Class of 1999
> > <[EMAIL PROTECTED]> UNIX System Administrator, nj.org
> >
> > _______________________________________________
> > evolution-hackers maillist - [EMAIL PROTECTED]
> > http://lists.ximian.com/mailman/listinfo/evolution-hackers
>
>
> _______________________________________________
> evolution-hackers maillist - [EMAIL PROTECTED]
> http://lists.ximian.com/mailman/listinfo/evolution-hackers
--
Russell Steinthal Columbia Law School, Class of 2002
<[EMAIL PROTECTED]> Columbia College, Class of 1999
<[EMAIL PROTECTED]> UNIX System Administrator, nj.org
_______________________________________________
evolution-hackers maillist - [EMAIL PROTECTED]
http://lists.ximian.com/mailman/listinfo/evolution-hackers
