Обсуждение: Returning non-terminated string in ECPG Informix-compatible function

Greetings, everyone!

While analyzing output of Svace static analyzer [1] I've found a bug.

In function intoasc(interval * i, char *str) from file 
src/interfaces/ecpg/compatlib/informix.c
we return a non-terminated string since we use memcpy on tmp which is 
itself NULL-teminated but
last zero byte is not copied.

The proposed solution is to use strcpy instead, since it is used in all 
other functions in informix.c.

The patch is attached.

[1] - https://svace.pages.ispras.ru/svace-website/en/

Oleg Tselebrovskiy, Postgres Pro

On Mon, Jan 29, 2024 at 2:17 PM <o.tselebrovskiy@postgrespro.ru> wrote:
>
> Greetings, everyone!
>
> While analyzing output of Svace static analyzer [1] I've found a bug.
>
> In function intoasc(interval * i, char *str) from file
> src/interfaces/ecpg/compatlib/informix.c
> we return a non-terminated string since we use memcpy on tmp which is
> itself NULL-teminated but
> last zero byte is not copied.
>
> The proposed solution is to use strcpy instead, since it is used in all
> other functions in informix.c.
>
> The patch is attached.
>
> [1] - https://svace.pages.ispras.ru/svace-website/en/
>

Can you please add a test case showcasing the bug? I see dttoasc()
uses strcpy(). So there's already a precedence.

--
Best Wishes,
Ashutosh Bapat

Here's the code for bug reproduction:
#include <stdio.h>
#include <stdlib.h>

EXEC SQL INCLUDE pgtypes_interval.h;
EXEC SQL INCLUDE ecpg_informix.h;

EXEC SQL BEGIN DECLARE SECTION;
    char dirty_str[100] = "aaaaaaaaa_bbbbbbbb_ccccccccc_ddddddddd_";
    interval *interval_ptr;
EXEC SQL END DECLARE SECTION;

int main()
{
    interval_ptr = (interval *) malloc(sizeof(interval));
    interval_ptr->time = 100000000;
    interval_ptr->month = 240;

    printf("dirty_str contents before intoasc: %s\n", dirty_str);
    intoasc(interval_ptr, dirty_str);
    printf("dirty_str contents after intoasc: %s\n", dirty_str);
    return 0;
}

And here's the output:

dirty_str contents before intoasc: 
aaaaaaaaa_bbbbbbbb_ccccccccc_ddddddddd_
dirty_str contents after intoasc: @ 20 years 1 min 40 
secscccc_ddddddddd_

I compiled it with following commands (provided for quicker 
reproduction):
/path/to/pgsql/bin/ecpg informix_bug_example.pgc
gcc -I/path/to/pgsql/include -c informix_bug_example.c
gcc -o informix_bug_example informix_bug_example.o -L/path/to/pgsql/lib 
-lecpg -lecpg_compat

I've also found at least one project that uses intoasc() in it - 
https://github.com/credativ/informix_fdw/

Oleg Tselebrovskiy, Postgres Pro

Greetings again.
I was looking through more static analyzer output and found another 
problem.
In ecpg/pgtypeslib/dt_common.c there are 4 calls of pgtypes_alloc.
This function uses calloc and returns NULL if OOM, but we don't check 
its
return value and immediately pass it to strcpy, which could lead to 
segfault.

I suggest adding a check for a return value since all other calls of
pgtypes_alloc are checked for NULL.

A proposed patch (with previous and current changes) is attached

Oleg Tselebrovskiy, Postgres Pro

On Thu, Feb 15, 2024 at 12:15:40PM +0700, Oleg Tselebrovskiy wrote:
> Greetings again.
> I was looking through more static analyzer output and found another problem.
> In ecpg/pgtypeslib/dt_common.c there are 4 calls of pgtypes_alloc.
> This function uses calloc and returns NULL if OOM, but we don't check its
> return value and immediately pass it to strcpy, which could lead to
> segfault.
>
> I suggest adding a check for a return value since all other calls of
> pgtypes_alloc are checked for NULL.

Right.

> @@ -654,7 +654,7 @@ intoasc(interval * i, char *str)
>      if (!tmp)
>          return -errno;
>
> -    memcpy(str, tmp, strlen(tmp));
> +    strcpy(str, tmp);

For this stuff, Ashutosh's point was to integrate a test case in the
patch.  With the pgc you have posted, most of the work is done, but
this should be added to src/interfaces/ecpg/test/sql/ to add some
automated coverage.  See the area for references showing how this is
achieved.

> @@ -2837,6 +2843,8 @@ PGTYPEStimestamp_defmt_scan(char **str, char *fmt, timestamp * d,
>              case 'T':
>                  pfmt++;
>                  tmp = pgtypes_alloc(strlen("%H:%M:%S") + strlen(pstr) + 1);
> +                if(!tmp)
> +                    return 1;

These are obviously wrong as pgtypes_alloc() could return NULL.
--
Michael

Thanks for review!

I added a regression test that is based on code from previous email

New patch is attached

Oleg Tselebrovskiy, Postgres Pro

On Thu, Feb 15, 2024 at 05:17:17PM +0700, Oleg Tselebrovskiy wrote:
> Thanks for review!

dt_common.c is quite amazing, the APIs that we have in it rely on
strcpy() but we have no idea of the length of the buffer string given
in input to store the result.  This would require breaking the
existing APIs or inventing new ones to be able to plug some safer
strlcpy() calls.  Not sure if it's really worth bothering.  For now,
I've applied the OOM checks on HEAD and the fix with the null
termination on all stable branches.
--
Michael