Ticket #4174 (new defect)

Opened 3 years ago

Last modified 3 years ago

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

Reported by: psprint Owned by:
Priority: major Milestone: Future Releases
Component: mc-core Version: master
Keywords: safety,underruns,string,utf8 Cc:
Blocked By: Blocking:
Branch state: no branch Votes for changeset:

Description

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.

Change History

comment:1 Changed 3 years ago by 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.

comment:2 in reply to: ↑ description Changed 3 years ago by 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.

comment:3 Changed 3 years ago by 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.

Changed 3 years ago by psprint

make indent

comment:4 Changed 3 years ago by 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)

comment:5 Changed 3 years ago by 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.

Note: See TracTickets for help on using tickets.