On Wed, 02 Sep 2009 21:16:03 +0400, Eugene wrote:

Enrico Tro"ger wrote:

...

On Wed, 02 Sep 2009 20:48:17 +0400, Eugene wrote:

...

Hi all.

The trouble is caused by unsafe usage of `strncpy' and long strings in Russian localization. I found that the stack overflow is caused by the following code (geany-plugins/geanygdb/src/gdb-ui-main.c : 366)

-------------------- 8< --------------------

if (text && disable_mnemonics) { gchar *p; gchar buf[32]; strncpy(buf, text, sizeof(buf)); for (p = buf; *p; p++) { if (*p == '_') { memmove(p, p + 1, strlen(p)); } } text = buf; }

-------------------- 8< --------------------

Obviously, 32 chars are enough for English localisation, but not for Russian one (which I am using). Quick fix:

... gchar buf[64]; strncpy(buf, text, sizeof(buf)-1); buf[sizeof(buf)-1] = '\0';

This is not really better. Increasing the buffer size only works as long as someone appears with another language which needs even more characters. Ok, at some point it gets unlikely if the buffer size is big enough but it's still ugly.

I suggest a more easy and secure approach (even being a little bit slower):

gchar *buf = g_strdup(text);

This is very unlikely to fail except there is no more free memory on the heap but well, in this case many many more things in Geany would go wrong...:)

Agree, g_strdup will be even better.

...

Btw, buf[sizeof(buf)-1]

is really wrong. Because sizeof(buf) is always 32 as it is a fixed sized char array. But you want to put the \0 at the end of the actual content not at the end of the buffer.

Putting \0 at the end of the buffer would be enough. If content is smaller than the buffer, strncpy itself will append \0.

Hmm, right. Still, IMO it's weird to set \o at sizeof(buf)-1.

Regards, Enrico

Am Mittwoch, den 02.09.2009, 19:10 +0200 schrieb Enrico Tröger:

On Wed, 02 Sep 2009 20:48:17 +0400, Eugene wrote:

...

Hi all.

The trouble is caused by unsafe usage of `strncpy' and long strings in Russian localization. I found that the stack overflow is caused by the following code (geany-plugins/geanygdb/src/gdb-ui-main.c : 366)

-------------------- 8< --------------------

if (text && disable_mnemonics) { gchar *p; gchar buf[32]; strncpy(buf, text, sizeof(buf)); for (p = buf; *p; p++) { if (*p == '_') { memmove(p, p + 1, strlen(p)); } } text = buf; }

-------------------- 8< --------------------

Obviously, 32 chars are enough for English localisation, but not for Russian one (which I am using). Quick fix:

... gchar buf[64]; strncpy(buf, text, sizeof(buf)-1); buf[sizeof(buf)-1] = '\0';

This is not really better. Increasing the buffer size only works as long as someone appears with another language which needs even more characters. Ok, at some point it gets unlikely if the buffer size is big enough but it's still ugly.

I suggest a more easy and secure approach (even being a little bit slower):

gchar *buf = g_strdup(text);

This is very unlikely to fail except there is no more free memory on the heap but well, in this case many many more things in Geany would go wrong...:)

Btw, buf[sizeof(buf)-1]

is really wrong. Because sizeof(buf) is always 32 as it is a fixed sized char array. But you want to put the \0 at the end of the actual content not at the end of the buffer.

Regards, Enrico

I fixed this issue as Enrico suggested. Thanks to you both for detecting and solving this issue. The fix will remain on my disk a bit because I've made other changes to gdb-ui-main.c which I have not yet finished. I will commit the fix as soon as possible.

Regards, Dominic