[patch] Improve safety of previous char fetching string functions.

[psprint]

Hi,
I was stumbled that e.g.: in completion engine there are calls such as:

        if (ti != text)
        {
            …
            prev_char = str_get_prev_char (ti)[0];
            …

while the length of the (potentially malformed) character is unknown and the function might underrun the buffer when peeking for some preceding bytes. I submit a patch that:

– extends each of the str_*_prev_char functions with an argument holding the beginning of the string,
– it then replaces the calls to unsafe g_utf8_prev_char with an underrun safe function g_utf8_find_prev_char,
– the function obtains the string starting address and takes care not to peek before it,
– in case of the string being ended without any next character found the functions return NULL,
– then all places in the completion engine and in usermenu.c etc. have been updated to follow this new paradigm,
– also a couple of bugs have been fixed, like e.g.: assuming of command position in input completion if a single character have been entered and a space.

I think that thanks to this safety improvements some occasional crashes in input completion will be over.

[angel_il]

Sebastian, attache file with broken words, please, for test current version mc and mc with your patches.

for all cases in your topic message, if this possible.

[andrew_b]

Again, again and again. Please reread our code guidelines.

Replying to psprint:

while the length of the (potentially malformed) character is unknown and the function might underrun the buffer when peeking for some preceding bytes. I submit a patch that:

– extends each of the str_*_prev_char functions with an argument holding the beginning of the string,

Tests for modified functions should be provided.

– in case of the string being ended without any next character found the functions return NULL,

in case of the string being started without any previous character found? I don't think it's a good solution. In non-utf strings previous chat is always valid.

– also a couple of bugs have been fixed, like e.g.: assuming of command position in input completion if a single character have been entered and a space.

It should be in the separated patch if possible.

I think that thanks to this safety improvements some occasional crashes in input completion will be over.

[psprint]

I've forgot to run make indent. I've thoroughly read the guide, don't know what's wrong?

Tests for modified functions should be provided.

Ok, I'll add some tests.

in case of the string being started without any previous character found?

Yes, that's what I've meant.

I don't think it's a good solution. In non-utf strings previous chat is always valid.

IMO any fetching of any bytes from before the legal buffer start is very hazardous. It's asking for problems… The code then works by depending on some outside checks to not dereference the pointer… A hot pointer I would say, all set up and ready to segfault… Even in non-utf8 strings it is illegal to grab a byte from before the allocated buffer.

It should be in the separated patch if possible.

I'll try to find some time, however I've already spent hours on this.

[psprint]

make indent

[andrew_b]

    /* Additional check to ensure no buffer underruns. */ 
    if (*text && *text < begin) 
        *text = NULL; 

If (*text < begin) is true, you're out of buffer. In this case test of (*text != NULL) is incorrect.

Let's read HACKING together.

Use explicit comparison in equality operators:

This is right:

void *p1, *p2;
int i1, i2;
char c1, c2;

if (p1 != NULL)
if (p2 == NULL)

if (i1 != 0)
if (i2 == 0)

if (c1 != '\0')
if (c2 == '\0')

This is wrong:

void *p1, *p2;
int i1, i2;
char c1, c2;

if (p1)
if (!p2)

if (i1)
if (!i2)

if (c1)
if (!c2)

[psprint]

I'm continuing discussion here after ​reply.

I meant that I've mixed the changes in completion_engine.c, so I whould have to unmerge. If it is required, then fine… Also, I've changed the *text NULL condition so that it isn't needed, by:

-    else
+    else if (*text > begin)
         (*text)--;
+    else
+        /* Buffer underrun. */
+        *text = NULL;

It's still in the multi search patch currently.