bug#6855: 24.0.50; Bug in tool bar label handling

bug#6855: 24.0.50; Bug in tool bar label handling

[Johan Bockgård]

There are some bugs in the handling of tool bar labels that can cause
Emacs to crash.



### gtkutil.c: update_frame_tool_bar ###

    char *label = SSDATA (PROP (TOOL_BAR_ITEM_LABEL));

Here we take string data out.



### keyboard.c: parse_tool_bar_item ###

      else if (EQ (key, QClabel))
        {
          /* `:label LABEL-STRING'.  */
          PROP (TOOL_BAR_ITEM_LABEL) = value;
          have_label = 1;
        }

But here we put an arbitrary object in.


...

  if (!have_label)

...
      char buf[64];
      EMACS_INT max_lbl = 2*tool_bar_max_label_size;
      Lisp_Object new_lbl;

      if (strlen (caption) < max_lbl && caption[0] != '\0')
        {
          strcpy (buf, caption);

tool-bar-max-label-size is a user variable, so this can mean a buffer
overflow.


...
      if (SCHARS (new_lbl) <= tool_bar_max_label_size)
        PROP (TOOL_BAR_ITEM_LABEL) = new_lbl;

If we came here but the branch is not taken, the label will be nil,
not a string.

bug#6855: 24.0.50; Bug in tool bar label handling

[Jan Djärv]

Johan Bockgård skrev 2010-08-14 14.04:
>
> There are some bugs in the handling of tool bar labels that can cause
> Emacs to crash.
>
>
>
> ### gtkutil.c: update_frame_tool_bar ###
>
>      char *label = SSDATA (PROP (TOOL_BAR_ITEM_LABEL));
>
> Here we take string data out.
>
>
>
> ### keyboard.c: parse_tool_bar_item ###
>
>        else if (EQ (key, QClabel))
>          {
>            /* `:label LABEL-STRING'.  */
>            PROP (TOOL_BAR_ITEM_LABEL) = value;
>            have_label = 1;
>          }
>
> But here we put an arbitrary object in.
>

We kind of assume people do the sensible thing and put in strings.  It is the
same as for help and image.  If Emacs crashes because somebody didn't put
in a string, that is actually a good thing IMHO.  The error becomes very
apparent then.

>
> ...
>
>    if (!have_label)
>
> ...
>        char buf[64];
>        EMACS_INT max_lbl = 2*tool_bar_max_label_size;
>        Lisp_Object new_lbl;
>
>        if (strlen (caption)<  max_lbl&&  caption[0] != '\0')
>          {
>            strcpy (buf, caption);
>
> tool-bar-max-label-size is a user variable, so this can mean a buffer
> overflow.
>
>
> ...
>        if (SCHARS (new_lbl)<= tool_bar_max_label_size)
>          PROP (TOOL_BAR_ITEM_LABEL) = new_lbl;
>
> If we came here but the branch is not taken, the label will be nil,
> not a string.
>

I have checked in a fix for those two.

Thanks,

	Jan D.

bug#6855: 24.0.50; Bug in tool bar label handling

[Andreas Schwab]

Jan Djärv <jan.h.d@swipnet.se> writes:

> We kind of assume people do the sensible thing and put in strings.  It is the
> same as for help and image.  If Emacs crashes because somebody didn't put
> in a string, that is actually a good thing IMHO.  The error becomes very
> apparent then.

I don't agree.  Emacs should be robust against type mismatches, crashing
is the worst possible reaction.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#6855: 24.0.50; Bug in tool bar label handling

[Jan Djärv]

Andreas Schwab skrev 2010-08-15 10.51:
> Jan Djärv<jan.h.d@swipnet.se>  writes:
>
>> We kind of assume people do the sensible thing and put in strings.  It is the
>> same as for help and image.  If Emacs crashes because somebody didn't put
>> in a string, that is actually a good thing IMHO.  The error becomes very
>> apparent then.
>
> I don't agree.  Emacs should be robust against type mismatches, crashing
> is the worst possible reaction.

If the documentation states that one should use STRING, and somebody puts in 
nil or a lambda expression or a symbol, that is a usage error.  Being robust 
against this kind of error by ignoring the faulty input just hides the error 
and makes people think it is OK to misuse things.  Better then to crash, that 
way action is usually taken at once.  Hidden errors can linger for years...

	Jan D.

bug#6855: 24.0.50; Bug in tool bar label handling

[Andreas Schwab]

Jan Djärv <jan.h.d@swipnet.se> writes:

> If the documentation states that one should use STRING, and somebody puts
> in nil or a lambda expression or a symbol, that is a usage error.

Sure.  But crashing is a bug.

> Better then to crash

No, never.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#6855: 24.0.50; Bug in tool bar label handling

[Eli Zaretskii]

> Date: Sun, 15 Aug 2010 12:21:37 +0200
> From: Jan Djärv <jan.h.d@swipnet.se>
> Cc: 6855@debbugs.gnu.org
> 
> If the documentation states that one should use STRING, and somebody puts in 
> nil or a lambda expression or a symbol, that is a usage error.  Being robust 
> against this kind of error by ignoring the faulty input just hides the error 
> and makes people think it is OK to misuse things.  Better then to crash, that 
> way action is usually taken at once.  Hidden errors can linger for years...

Crash or hide are not the only alternatives.  You can signal an error,
for instance.  Sometimes doing so is not a good idea, like in the
middle of redisplay (because displaying the error message reenters
redisplay again, and you have an infinite loop on your hands).  For
these situations, the solution is to display something prominent and
acutely visible instead of the invalid data, so that it stands out and
catches the user's eye.  For example, if the menu item is bad, display
something like "!!??GARBLED ITEM??!!" instead.

In general, I agree with Andreas: it is better not to crash, if we can
avoid that with a reasonable effort.