[2962] in BarnOwl Developers
Re: [PATCH 2/2] Don't pass limited buffer lengths to owl_keybinding_tostring
daemon@ATHENA.MIT.EDU (Jason Gross)
Fri Mar 11 00:24:50 2011
In-Reply-To: <1299790123-17519-2-git-send-email-davidben@mit.edu>
From: Jason Gross <jasongross9@gmail.com>
Date: Fri, 11 Mar 2011 00:24:18 -0500
To: David Benjamin <davidben@mit.edu>
Cc: barnowl-dev@mit.edu
--000e0cdf1b763c372f049e2e2e33
Content-Type: text/plain; charset=ISO-8859-1
You should probably remove the now-inaccurate comments about returning 0 on
success.
Questions about the code:
Where does the 11 in 11-strlen(kbstr) come from?
For not hardcoding 64 (in this method, at least), is there a reason to not
have owl_keypress_tostring return a char* (or GString)?
-Jason
On Thu, Mar 10, 2011 at 3:48 PM, David Benjamin <davidben@mit.edu> wrote:
> The keymap tostring code still probably should be cleaned up though.
> ---
> keybinding.c | 27 +++++++++++++--------------
> keymap.c | 13 +++++++------
> 2 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/keybinding.c b/keybinding.c
> index a5c977f..9884251 100644
> --- a/keybinding.c
> +++ b/keybinding.c
> @@ -85,26 +85,25 @@ void owl_keybinding_execute(const owl_keybinding *kb,
> int j)
> }
>
> /* returns 0 on success */
> -int owl_keybinding_stack_tostring(int *j, int len, char *buff, int
> bufflen)
> +char *owl_keybinding_stack_tostring(int *j, int len)
> {
> - char *pos = buff;
> - int rem = bufflen;
> - int i, n;
> -
> - for (i=0; i < len; i++) {
> - owl_keypress_tostring(j[i], 0, pos, rem-1);
> - if (i < len - 1) strcat(pos, " ");
> - n = strlen(pos);
> - pos += n;
> - rem -= n;
> + GString *string;
> + char keypress[64]; /* TODO: don't hard-code a value here. */
> + int i;
> +
> + string = g_string_new("");
> + for (i = 0; i < len; i++) {
> + owl_keypress_tostring(j[i], 0, keypress, sizeof(keypress));
> + g_string_append(string, keypress);
> + if (i < len - 1) g_string_append_c(string, ' ');
> }
> - return 0;
> + return g_string_free(string, false);
> }
>
> /* returns 0 on success */
> -int owl_keybinding_tostring(const owl_keybinding *kb, char *buff, int
> bufflen)
> +char *owl_keybinding_tostring(const owl_keybinding *kb)
> {
> - return owl_keybinding_stack_tostring(kb->keys, kb->len, buff, bufflen);
> + return owl_keybinding_stack_tostring(kb->keys, kb->len);
> }
>
> const char *owl_keybinding_get_desc(const owl_keybinding *kb)
> diff --git a/keymap.c b/keymap.c
> index 13a07dc..176ad47 100644
> --- a/keymap.c
> +++ b/keymap.c
> @@ -143,15 +143,16 @@ static void _owl_keymap_format_bindings(const
> owl_keymap *km, owl_fmtext *fm)
>
> nbindings = owl_list_get_size(&km->bindings);
> for (i=0; i<nbindings; i++) {
> - char buff[100];
> + char *kbstr;
> const owl_cmd *cmd;
> const char *tmpdesc, *desc = "";
>
> kb = owl_list_get_element(&km->bindings, i);
> - owl_keybinding_tostring(kb, buff, 100);
> + kbstr = owl_keybinding_tostring(kb);
> owl_fmtext_append_normal(fm, OWL_TABSTR);
> - owl_fmtext_append_normal(fm, buff);
> - owl_fmtext_append_spaces(fm, 11-strlen(buff));
> + owl_fmtext_append_normal(fm, kbstr);
> + owl_fmtext_append_spaces(fm, 11-strlen(kbstr));
> + g_free(kbstr);
> owl_fmtext_append_normal(fm, " - ");
> if (kb->desc && *kb->desc) {
> desc = kb->desc;
> @@ -319,8 +320,8 @@ int owl_keyhandler_process(owl_keyhandler *kh,
> owl_input j)
>
> void owl_keyhandler_invalidkey(owl_keyhandler *kh)
> {
> - char kbbuff[500];
> - owl_keybinding_stack_tostring(kh->kpstack, kh->kpstackpos+1, kbbuff,
> 500);
> + char *kbbuff = owl_keybinding_stack_tostring(kh->kpstack,
> kh->kpstackpos+1);
> owl_function_makemsg("'%s' is not a valid key in this context.",
> kbbuff);
> + g_free(kbbuff);
> owl_keyhandler_reset(kh);
> }
> --
> 1.7.3.3.399.g06ddf
>
>
--000e0cdf1b763c372f049e2e2e33
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
You should probably remove the now-inaccurate comments about returning 0 on=
success.<div><br></div><div>Questions about the code:</div><div><span clas=
s=3D"Apple-style-span" style=3D"border-collapse: collapse; font-family: ari=
al, sans-serif; font-size: 13px; ">Where does the 11 in 11-strlen(kbstr) co=
me from?</span></div>
<div><font class=3D"Apple-style-span" face=3D"arial, sans-serif"><span clas=
s=3D"Apple-style-span" style=3D"border-collapse: collapse;">For not hardcod=
ing 64 (in this method, at least), is there a reason to not have owl_keypre=
ss_tostring return a char* (or GString)?<br>
</span></font><br></div><div>-Jason</div><div><br><div class=3D"gmail_quote=
">On Thu, Mar 10, 2011 at 3:48 PM, David Benjamin <span dir=3D"ltr"><<a =
href=3D"mailto:davidben@mit.edu">davidben@mit.edu</a>></span> wrote:<br>=
<blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p=
x #ccc solid;padding-left:1ex;">
The keymap tostring code still probably should be cleaned up though.<br>
---<br>
=A0keybinding.c | =A0 27 +++++++++++++--------------<br>
=A0keymap.c =A0 =A0 | =A0 13 +++++++------<br>
=A02 files changed, 20 insertions(+), 20 deletions(-)<br>
<br>
diff --git a/keybinding.c b/keybinding.c<br>
index a5c977f..9884251 100644<br>
--- a/keybinding.c<br>
+++ b/keybinding.c<br>
@@ -85,26 +85,25 @@ void owl_keybinding_execute(const owl_keybinding *kb, i=
nt j)<br>
=A0}<br>
<br>
=A0/* returns 0 on success */<br>
-int owl_keybinding_stack_tostring(int *j, int len, char *buff, int bufflen=
)<br>
+char *owl_keybinding_stack_tostring(int *j, int len)<br>
=A0{<br>
- =A0char *pos =3D buff;<br>
- =A0int =A0 rem =3D bufflen;<br>
- =A0int =A0 i, n;<br>
-<br>
- =A0for (i=3D0; i < len; i++) {<br>
- =A0 =A0owl_keypress_tostring(j[i], 0, pos, rem-1);<br>
- =A0 =A0if (i < len - 1) strcat(pos, " ");<br>
- =A0 =A0n =3D strlen(pos);<br>
- =A0 =A0pos +=3D n;<br>
- =A0 =A0rem -=3D n;<br>
+ =A0GString *string;<br>
+ =A0char keypress[64]; =A0/* TODO: don't hard-code a value here. */<br=
>
+ =A0int =A0i;<br>
+<br>
+ =A0string =3D g_string_new("");<br>
+ =A0for (i =3D 0; i < len; i++) {<br>
+ =A0 =A0owl_keypress_tostring(j[i], 0, keypress, sizeof(keypress));<br>
+ =A0 =A0g_string_append(string, keypress);<br>
+ =A0 =A0if (i < len - 1) g_string_append_c(string, ' ');<br>
=A0 }<br>
- =A0return 0;<br>
+ =A0return g_string_free(string, false);<br>
=A0}<br>
<br>
=A0/* returns 0 on success */<br>
-int owl_keybinding_tostring(const owl_keybinding *kb, char *buff, int buff=
len)<br>
+char *owl_keybinding_tostring(const owl_keybinding *kb)<br>
=A0{<br>
- =A0return owl_keybinding_stack_tostring(kb->keys, kb->len, buff, bu=
fflen);<br>
+ =A0return owl_keybinding_stack_tostring(kb->keys, kb->len);<br>
=A0}<br>
<br>
=A0const char *owl_keybinding_get_desc(const owl_keybinding *kb)<br>
diff --git a/keymap.c b/keymap.c<br>
index 13a07dc..176ad47 100644<br>
--- a/keymap.c<br>
+++ b/keymap.c<br>
@@ -143,15 +143,16 @@ static void _owl_keymap_format_bindings(const owl_key=
map *km, owl_fmtext *fm)<br>
<br>
=A0 nbindings =3D owl_list_get_size(&km->bindings);<br>
=A0 for (i=3D0; i<nbindings; i++) {<br>
- =A0 =A0char buff[100];<br>
+ =A0 =A0char *kbstr;<br>
=A0 =A0 const owl_cmd *cmd;<br>
=A0 =A0 const char *tmpdesc, *desc =3D "";<br>
<br>
=A0 =A0 kb =3D owl_list_get_element(&km->bindings, i);<br>
- =A0 =A0owl_keybinding_tostring(kb, buff, 100);<br>
+ =A0 =A0kbstr =3D owl_keybinding_tostring(kb);<br>
=A0 =A0 owl_fmtext_append_normal(fm, OWL_TABSTR);<br>
- =A0 =A0owl_fmtext_append_normal(fm, buff);<br>
- =A0 =A0owl_fmtext_append_spaces(fm, 11-strlen(buff));<br>
+ =A0 =A0owl_fmtext_append_normal(fm, kbstr);<br>
+ =A0 =A0owl_fmtext_append_spaces(fm, 11-strlen(kbstr));<br>
+ =A0 =A0g_free(kbstr);<br>
=A0 =A0 owl_fmtext_append_normal(fm, " - ");<br>
=A0 =A0 if (kb->desc && *kb->desc) {<br>
=A0 =A0 =A0 desc =3D kb->desc;<br>
@@ -319,8 +320,8 @@ int owl_keyhandler_process(owl_keyhandler *kh, owl_inpu=
t j)<br>
<br>
=A0void owl_keyhandler_invalidkey(owl_keyhandler *kh)<br>
=A0{<br>
- =A0 =A0char kbbuff[500];<br>
- =A0 =A0owl_keybinding_stack_tostring(kh->kpstack, kh->kpstackpos+1,=
kbbuff, 500);<br>
+ =A0 =A0char *kbbuff =3D owl_keybinding_stack_tostring(kh->kpstack, kh-=
>kpstackpos+1);<br>
=A0 =A0 owl_function_makemsg("'%s' is not a valid key in this=
context.", kbbuff);<br>
+ =A0 =A0g_free(kbbuff);<br>
=A0 =A0 owl_keyhandler_reset(kh);<br>
=A0}<br>
<font color=3D"#888888">--<br>
1.7.3.3.399.g06ddf<br>
<br>
</font></blockquote></div><br></div>
--000e0cdf1b763c372f049e2e2e33--
