Thread: [PATCH] Fix out-of-bouds access (src/common/wchar.c)

At Tue, 15 Feb 2022 09:17:34 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> Per Coverity.

Thanks for the source:)

> Like the function pg_encoding_max_length_sql
> (src/backend/utils/mb/mbutils.c)
> Only assertion is insufficient to avoid accessing array out-of-bounds.
> 
> This bug is live according Coverity at function: pg_verify_mbstr_len
> (src/backend/utils/mb/mbutils.c)
> CID 1469870 (#1 of 1): Out-of-bounds access (OVERRUN)7. overrun-call:
> Overrunning
> callee's array of size 42 by passing argument src_encoding (which evaluates
> to 63) in call to pg_verify_mbstr_len. [show details
>
<https://scan6.scan.coverity.com/eventId=32693869-7&modelId=32693869-0&fileInstanceId=131415642&filePath=%2Fdll%2Fpostgres%2Fpostgres%2Fsrc%2Fbackend%2Futils%2Fmb%2Fmbutils.c&fileStart=1546&fileEnd=1605>

It returns 400..

> ]
> 633        retval = pg_verify_mbstr_len(src_encoding, src_str, len, false);
> 634
> 
> Trivial patch attached.

Mmm? If the assert doesn't work, there should be inconcsistency
between pg_enc and pg_wchar_table. But AFAICS they are consistent.

The patch:
 pg_encoding_max_length(int encoding)
 {
-    Assert(PG_VALID_ENCODING(encoding));
-
-    return pg_wchar_table[encoding].maxmblen;
+    if (PG_VALID_ENCODING(encoding))
+        return pg_wchar_table[encoding].maxmblen;
+    else
+        return -1;

Returning -1 for invalid encoding is, I think, flat wrong.


> 633        retval = pg_verify_mbstr_len(src_encoding, src_str, len, false);

The src_encoding comes from pg_char_to_encoding. It returns
pg_encname_tbl[n]->encoding, which seems to reutrn a value of
pg_enc. the max value of which is 42. So I don't come up with a
hypothesis that makes it possible for now..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 16 Feb 2022 09:29:20 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> > > ]
> > > 633        retval = pg_verify_mbstr_len(src_encoding, src_str, len,
> > false);
> > > 634
> > >
> > > Trivial patch attached.
> >
> > Mmm? If the assert doesn't work, there should be inconcsistency
> > between pg_enc and pg_wchar_table. But AFAICS they are consistent.
> >
> The consistency is between pg_encname_tbl and pc_enc, and AFAICS are
> consistent.

..Yeah, right.

> > The patch:
> >  pg_encoding_max_length(int encoding)
> >  {
> > -       Assert(PG_VALID_ENCODING(encoding));
> > -
> > -       return pg_wchar_table[encoding].maxmblen;
> > +       if (PG_VALID_ENCODING(encoding))
> > +               return pg_wchar_table[encoding].maxmblen;
> > +       else
> > +               return -1;
> >
> > Returning -1 for invalid encoding is, I think, flat wrong.
> >
> Ok, if -1 is wrong, what should the value of return if
> somebody calling this function like:
> pg_encoding_max_length(63);

Should result in assertion failure, I think.  If that fails, the
caller side is anyhow broken.  On the other hand we haven't had I'll
dig into that further.

> Of course, with patch applied, because with original code
> has memory corruption, if built and run without DEBUG.

So we don't assume corruption in production build.  It should be
logically guaranteed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

(Sorry for the broken mail...)

At Wed, 16 Feb 2022 09:29:20 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> > > ]
> > > 633        retval = pg_verify_mbstr_len(src_encoding, src_str, len,
> > false);
> > > 634
> > >
> > > Trivial patch attached.
> >
> > Mmm? If the assert doesn't work, there should be inconcsistency
> > between pg_enc and pg_wchar_table. But AFAICS they are consistent.
> >
> The consistency is between pg_encname_tbl and pc_enc, and AFAICS are
> consistent.

..Yeah, right.

> > The patch:
> >  pg_encoding_max_length(int encoding)
> >  {
> > -       Assert(PG_VALID_ENCODING(encoding));
> > -
> > -       return pg_wchar_table[encoding].maxmblen;
> > +       if (PG_VALID_ENCODING(encoding))
> > +               return pg_wchar_table[encoding].maxmblen;
> > +       else
> > +               return -1;
> >
> > Returning -1 for invalid encoding is, I think, flat wrong.
> >
> Ok, if -1 is wrong, what should the value of return if
> somebody calling this function like:
> pg_encoding_max_length(63);

Should result in assertion failure, I think.  If that fails, the
caller side is anyhow broken.  On the other hand we haven't had a
complain about that, maybe.

> Of course, with patch applied, because with original code
> has memory corruption, if built and run without DEBUG.

So we don't assume corruption in production build.  It should be
logically guaranteed.

I'll dig into that further.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 17 Feb 2022 14:58:38 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> (Sorry for the broken mail...)
> 
> At Wed, 16 Feb 2022 09:29:20 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> > > The patch:
> > >  pg_encoding_max_length(int encoding)
> > >  {
> > > -       Assert(PG_VALID_ENCODING(encoding));
> > > -
> > > -       return pg_wchar_table[encoding].maxmblen;
> > > +       if (PG_VALID_ENCODING(encoding))
> > > +               return pg_wchar_table[encoding].maxmblen;
> > > +       else
> > > +               return -1;
> > >
> > > Returning -1 for invalid encoding is, I think, flat wrong.
> > >
> > Ok, if -1 is wrong, what should the value of return if
> > somebody calling this function like:
> > pg_encoding_max_length(63);
> 
> Should result in assertion failure, I think.  If that fails, the
> caller side is anyhow broken.  On the other hand we haven't had a
> complain about that, maybe.
> 
> > Of course, with patch applied, because with original code
> > has memory corruption, if built and run without DEBUG.
> 
> So we don't assume corruption in production build.  It should be
> logically guaranteed.
> 
> I'll dig into that further.

The number comes from pg_char_to_encoding, which is the internal
function of the SQL function PG_char_encoding(). It looks fine but I
can confirm that for all possible encoding names in pg_encname_tbl[].

=# select * from (
select e as name, PG_char_to_encoding(e) enc
from unnest(array['abc', 'alt', 'big5', 'euccn', 'eucjis2004', 'eucjp',
    'euckr', 'euctw', 'gb18030', 'gbk', 'iso88591', 'iso885910', 'iso885913',
    'iso885914', 'iso885915', 'iso885916', 'iso88592', 'iso88593', 'iso88594',
    'iso88595', 'iso88596', 'iso88597', 'iso88598', 'iso88599', 'johab',
    'koi8', 'koi8r', 'koi8u', 'latin1', 'latin10', 'latin2', 'latin3',
    'latin4', 'latin5', 'latin6', 'latin7', 'latin8', 'latin9', 'mskanji',
    'muleinternal', 'shiftjis', 'shiftjis2004', 'sjis', 'sqlascii', 'tcvn',
    'tcvn5712', 'uhc', 'unicode', 'utf8', 'vscii', 'win', 'win1250', 'win1251',
    'win1252', 'win1253', 'win1254', 'win1255', 'win1256', 'win1257',
    'win1258', 'win866', 'win874', 'win932', 'win936', 'win949', 'win950',
    'windows1250', 'windows1251', 'windows1252', 'windows1253', 'windows1254',
    'windows1255', 'windows1256', 'windows1257', 'windows1258', 'windows866',
    'windows874', 'windows932', 'windows936', 'windows949', 'windows950',
    'hoge']) as e) as t where enc < 0 or enc > 41;
 name | enc 
------+-----
 hoge |  -1
(1 row)

So, the function doesn't return 63 for all registered names and wrong
names.

So other possibilities I can think of are..

- Someone had broken pg_encname_tbl[]

- Cosmic ray hit, or ill memory cell.

- Coverity worked wrong way.


Could you show the workload for the Coverity warning here?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 17 Feb 2022 15:51:26 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> - Cosmic ray hit, or ill memory cell.

63 (0x3f) cannot be less than 42(0x2a) by one-bit flip.  So the
possibility of cosmic ray would be quite low.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Feb 17, 2022 at 03:51:26PM +0900, Kyotaro Horiguchi wrote:
> At Thu, 17 Feb 2022 14:58:38 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > (Sorry for the broken mail...)
> > > >
> > > Ok, if -1 is wrong, what should the value of return if
> > > somebody calling this function like:
> > > pg_encoding_max_length(63);
> > 
> > Should result in assertion failure, I think.  If that fails, the
> > caller side is anyhow broken.  On the other hand we haven't had a
> > complain about that, maybe.
> > 
> > > Of course, with patch applied, because with original code
> > > has memory corruption, if built and run without DEBUG.
> > 
> > So we don't assume corruption in production build.  It should be
> > logically guaranteed.
> > 
> > I'll dig into that further.
> 
> The number comes from pg_char_to_encoding, which is the internal
> function of the SQL function PG_char_encoding(). It looks fine but I
> can confirm that for all possible encoding names in pg_encname_tbl[].
> 
> =# select * from (
> select e as name, PG_char_to_encoding(e) enc
> from unnest(array['abc', 'alt', 'big5', 'euccn', 'eucjis2004', 'eucjp',
>     'euckr', 'euctw', 'gb18030', 'gbk', 'iso88591', 'iso885910', 'iso885913',
>     'iso885914', 'iso885915', 'iso885916', 'iso88592', 'iso88593', 'iso88594',
>     'iso88595', 'iso88596', 'iso88597', 'iso88598', 'iso88599', 'johab',
>     'koi8', 'koi8r', 'koi8u', 'latin1', 'latin10', 'latin2', 'latin3',
>     'latin4', 'latin5', 'latin6', 'latin7', 'latin8', 'latin9', 'mskanji',
>     'muleinternal', 'shiftjis', 'shiftjis2004', 'sjis', 'sqlascii', 'tcvn',
>     'tcvn5712', 'uhc', 'unicode', 'utf8', 'vscii', 'win', 'win1250', 'win1251',
>     'win1252', 'win1253', 'win1254', 'win1255', 'win1256', 'win1257',
>     'win1258', 'win866', 'win874', 'win932', 'win936', 'win949', 'win950',
>     'windows1250', 'windows1251', 'windows1252', 'windows1253', 'windows1254',
>     'windows1255', 'windows1256', 'windows1257', 'windows1258', 'windows866',
>     'windows874', 'windows932', 'windows936', 'windows949', 'windows950',
>     'hoge']) as e) as t where enc < 0 or enc > 41;
>  name | enc 
> ------+-----
>  hoge |  -1
> (1 row)
> 
> So, the function doesn't return 63 for all registered names and wrong
> names.
> 
> So other possibilities I can think of are..
> - Someone had broken pg_encname_tbl[]
> - Cosmic ray hit, or ill memory cell.
> - Coverity worked wrong way.
> 
> Could you show the workload for the Coverity warning here?

The 63 upthread was hypothetical right?  pg_encoding_max_length() shouldn't be
called with user-dependent data (unlike pg_encoding_max_length_sql()), so I
also don't see any value spending cycles in release builds.  The error should
only happen with bogus code, and assert builds are there to avoid that, or
corrupted memory and in that case we can't make any promise.

At Thu, 17 Feb 2022 15:50:09 +0800, Julien Rouhaud <rjuju123@gmail.com> wrote in 
> On Thu, Feb 17, 2022 at 03:51:26PM +0900, Kyotaro Horiguchi wrote:
> > So, the function doesn't return 63 for all registered names and wrong
> > names.
> > 
> > So other possibilities I can think of are..
> > - Someone had broken pg_encname_tbl[]
> > - Cosmic ray hit, or ill memory cell.
> > - Coverity worked wrong way.
> > 
> > Could you show the workload for the Coverity warning here?
> 
> The 63 upthread was hypothetical right?  pg_encoding_max_length() shouldn't be

I understand that Coverity complaind pg_verify_mbstr_len is fed with
encoding = 63 by length_in_encoding.  I don't know what made Coverity
think so.

> called with user-dependent data (unlike pg_encoding_max_length_sql()), so I
> also don't see any value spending cycles in release builds.  The error should
> only happen with bogus code, and assert builds are there to avoid that, or
> corrupted memory and in that case we can't make any promise.

Well, It's more or less what I wanted to say. Thanks.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Feb 17, 2022 at 05:24:58PM +0900, Kyotaro Horiguchi wrote:
> At Thu, 17 Feb 2022 15:50:09 +0800, Julien Rouhaud <rjuju123@gmail.com> wrote in 
> > On Thu, Feb 17, 2022 at 03:51:26PM +0900, Kyotaro Horiguchi wrote:
> > > So, the function doesn't return 63 for all registered names and wrong
> > > names.
> > > 
> > > So other possibilities I can think of are..
> > > - Someone had broken pg_encname_tbl[]
> > > - Cosmic ray hit, or ill memory cell.
> > > - Coverity worked wrong way.
> > > 
> > > Could you show the workload for the Coverity warning here?
> > 
> > The 63 upthread was hypothetical right?  pg_encoding_max_length() shouldn't be
> 
> I understand that Coverity complaind pg_verify_mbstr_len is fed with
> encoding = 63 by length_in_encoding.  I don't know what made Coverity
> think so.

Not sure either.  As you said this assumes that pg_char_to_encoding() can
return something higher than _PG_LAST_ENCODING_ and I also fail to see how that
could happen.

> On 17 Feb 2022, at 13:19, Ranier Vilela <ranier.vf@gmail.com> wrote:

> 1. One #ifdef with a mistake, the correct is _WIN32 and not WIN32.

Can you elaborate on this, we are using WIN32 pretty extensively in the code:

 $ git grep "if[n]\{0,1\}def WIN32$"|wc -l
     511
 $ git grep "if[n]\{0,1\}def _WIN32$"|wc -l
       2

The _WIN32 cases are in the same ECPG testcase.

Why would _WIN32 be correct in this case?

--
Daniel Gustafsson        https://vmware.com/

> On 17 Feb 2022, at 13:59, Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em qui., 17 de fev. de 2022 às 09:52, Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> escreveu:
> > On 17 Feb 2022, at 13:19, Ranier Vilela <ranier.vf@gmail.com <mailto:ranier.vf@gmail.com>> wrote:
>
> > 1. One #ifdef with a mistake, the correct is _WIN32 and not WIN32.
>
> Can you elaborate on this, we are using WIN32 pretty extensively in the code:
>
>  $ git grep "if[n]\{0,1\}def WIN32$"|wc -l
>      511
>  $ git grep "if[n]\{0,1\}def _WIN32$"|wc -l
>        2
>
> The _WIN32 cases are in the same ECPG testcase.
>
> Why would _WIN32 be correct in this case?
> Sorry, my fault.
>
> I only use _WIN32 and I jumped to conclusions very quickly.

Question remains though, should src/interfaces/ecpg/test/sql/sqlda.pgc really
be using WIN32 and not _WIN32, or doesn't it matter?  (or does it only matter
for consistency?) WIN32 and _WIN32 aren't very informative searchterms to use
for finding more information.

--
Daniel Gustafsson        https://vmware.com

Daniel Gustafsson <daniel@yesql.se> writes:
> Question remains though, should src/interfaces/ecpg/test/sql/sqlda.pgc really
> be using WIN32 and not _WIN32, or doesn't it matter?  (or does it only matter
> for consistency?) WIN32 and _WIN32 aren't very informative searchterms to use
> for finding more information.

I find this in src/include/port/win32.h:

/*
 * We always rely on the WIN32 macro being set by our build system,
 * but _WIN32 is the compiler pre-defined macro. So make sure we define
 * WIN32 whenever _WIN32 is set, to facilitate standalone building.
 */
#if defined(_WIN32) && !defined(WIN32)
#define WIN32
#endif

So for most of our code it shouldn't matter.  However, I'm not sure
that the ECPG test cases include our port.h --- they probably shouldn't
if they're to reflect actual use-cases.  [ pokes around... ]  See
517bf2d91 which added this code.

            regards, tom lane

> On 17 Feb 2022, at 16:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Question remains though, should src/interfaces/ecpg/test/sql/sqlda.pgc really
>> be using WIN32 and not _WIN32, or doesn't it matter?  (or does it only matter
>> for consistency?) WIN32 and _WIN32 aren't very informative searchterms to use
>> for finding more information.
>
> I find this in src/include/port/win32.h:
>
> /*
> * We always rely on the WIN32 macro being set by our build system,
> * but _WIN32 is the compiler pre-defined macro. So make sure we define
> * WIN32 whenever _WIN32 is set, to facilitate standalone building.
> */
> #if defined(_WIN32) && !defined(WIN32)
> #define WIN32
> #endif
>
> So for most of our code it shouldn't matter.  However, I'm not sure
> that the ECPG test cases include our port.h --- they probably shouldn't
> if they're to reflect actual use-cases.  [ pokes around... ]  See
> 517bf2d91 which added this code.

Judging by the info in the Stack Overflow post and the commit message for
517bf2d91 it's seems perfectly correct to use _WIN32 in the ECPG test.  Thanks!

--
Daniel Gustafsson        https://vmware.com/