Out out bounds array access in ibm iconv bits

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Out out bounds array access in ibm iconv bits

Jeff Law

So AFAICT this is a true error in the ibm iconv code spotted by Martin's
improvements to the out of bounds array indexing warning on the gcc trunk.

Building trunk glibc with a trunk gcc (any target AFAICT) on results in
a warning like this:

In file included from ibm1364.c:393,
                 from ibm1390.c:35:
../iconv/skeleton.c: In function ‘gconv’:
../iconv/loop.c:446:274: error: array subscript 1 is outside array
bounds of ‘unsigned char[4]’ [-Werror=array-bounds]
       BODY


There's a truly *horrid* maze of twisty #defines in play here.  After
preprocessing it looks like this:


static inline int
__attribute ((always_inline))
to_ibm1390_single (struct __gconv_step *step,
   struct __gconv_step_data *step_data,
   const unsigned char **inptrp, const unsigned char *inend,
   unsigned char **outptrp, unsigned char *outend,
   size_t *irreversible , int *curcsp)
{
  mbstate_t *state = step_data->__statep;
  int flags = step_data->__flags;
  int result = __GCONV_OK;
  unsigned char bytebuf[4];
[ ... ]
  inptr = bytebuf;
[ ... ]
    { uint32_t ch = *((const uint32_t *) (inptr)); if (__builtin_expect
((ch >= 0xffffffff), 0)) { { if (((ch) >> 7) == (0xe0000 >> 7)) { inptr
+= 4; continue; } }; if (! (irreversible != ((void *)0) && (flags &
__GCONV_IGNORE_ERRORS))) { result = __GCONV_ILLEGAL_INPUT; break; }
++*irreversible; inptr += 4; continue; } { const struct combine *cmbp =
__ucs4_combined_to_ibm1390db; while (cmbp->res1 < ch) ++cmbp; if
(cmbp->res1 == ch && inptr + 4 < inend) { uint32_t ch_next = *((const
uint32_t *) (inptr + 4));

[ Insanely long line truncated ]

Note the dereference *((const uint32_t *) (inptr + 4))

Boom, that's out of bounds.  inptr points to bytebuf which is just 4
bytes in size, thus *(inptr+4) is bad.  Now perhaps there's something
buried in this mess that prevents the code in question from ever
executing, but I don't see what it would be.  The inptr + 4 < inend
guard seems like it might, but inend is an incoming parameter, so the
condition doesn't really tell the compiler anything useful.  In fact it
looks like we're comparing pointers to two completely unrelated objects.


Anyway, I'm heading out on PTO shortly.  So someone else will need to
pick up the ball here.

Jeff


Reply | Threaded
Open this post in threaded view
|

Re: Out out bounds array access in ibm iconv bits

Andreas Schwab
On Jul 23 2018, Jeff Law <[hidden email]> wrote:

> So AFAICT this is a true error in the ibm iconv code spotted by Martin's
> improvements to the out of bounds array indexing warning on the gcc trunk.
>
> Building trunk glibc with a trunk gcc (any target AFAICT) on results in
> a warning like this:
>
> In file included from ibm1364.c:393,
>                  from ibm1390.c:35:
> ../iconv/skeleton.c: In function ‘gconv’:
> ../iconv/loop.c:446:274: error: array subscript 1 is outside array
> bounds of ‘unsigned char[4]’ [-Werror=array-bounds]
>        BODY

I think this should fix it:

diff --git a/iconvdata/ibm1364.c b/iconvdata/ibm1364.c
index b833273aa8..9a40940ccf 100644
--- a/iconvdata/ibm1364.c
+++ b/iconvdata/ibm1364.c
@@ -296,6 +296,7 @@ enum
 
 /* Next, define the other direction.  */
 #define MIN_NEEDED_INPUT MIN_NEEDED_TO
+#define MAX_NEEDED_INPUT   MAX_NEEDED_TO
 #define MIN_NEEDED_OUTPUT MIN_NEEDED_FROM
 #define MAX_NEEDED_OUTPUT MAX_NEEDED_FROM
 #define LOOPFCT TO_LOOP

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix out-of-bounds access in IBM-1360 converter

Andreas Schwab
In reply to this post by Jeff Law
The IBM-1360 converter can consume/produce two UCS4 characters in each
loop.

        * iconvdata/ibm1364.c (MAX_NEEDED_OUTPUT) [FROM_LOOP]: Define.
        (MAX_NEEDED_INPUT) [TO_LOOP]: Define.
---
 iconvdata/ibm1364.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/iconvdata/ibm1364.c b/iconvdata/ibm1364.c
index b833273aa8..517fe60813 100644
--- a/iconvdata/ibm1364.c
+++ b/iconvdata/ibm1364.c
@@ -150,6 +150,7 @@ enum
 #define MIN_NEEDED_INPUT   MIN_NEEDED_FROM
 #define MAX_NEEDED_INPUT   MAX_NEEDED_FROM
 #define MIN_NEEDED_OUTPUT MIN_NEEDED_TO
+#define MAX_NEEDED_OUTPUT MAX_NEEDED_TO
 #define LOOPFCT FROM_LOOP
 #define BODY \
   {      \
@@ -296,6 +297,7 @@ enum
 
 /* Next, define the other direction.  */
 #define MIN_NEEDED_INPUT MIN_NEEDED_TO
+#define MAX_NEEDED_INPUT   MAX_NEEDED_TO
 #define MIN_NEEDED_OUTPUT MIN_NEEDED_FROM
 #define MAX_NEEDED_OUTPUT MAX_NEEDED_FROM
 #define LOOPFCT TO_LOOP
--
2.18.0


--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix out-of-bounds access in IBM-1360 converter

Jeff Law
On 07/24/2018 07:24 AM, Andreas Schwab wrote:
> The IBM-1360 converter can consume/produce two UCS4 characters in each
> loop.
>
> * iconvdata/ibm1364.c (MAX_NEEDED_OUTPUT) [FROM_LOOP]: Define.
> (MAX_NEEDED_INPUT) [TO_LOOP]: Define.
Yes, that fixes the problem.  Thanks!

jeff
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix out-of-bounds access in IBM-1360 converter

Carlos O'Donell-6
In reply to this post by Andreas Schwab
On 07/24/2018 09:24 AM, Andreas Schwab wrote:
> The IBM-1360 converter can consume/produce two UCS4 characters in each
> loop.
>
> * iconvdata/ibm1364.c (MAX_NEEDED_OUTPUT) [FROM_LOOP]: Define.
> (MAX_NEEDED_INPUT) [TO_LOOP]: Define.

Confirmed. In both ibm1390 and ibm1399 we have combining characters
which are two UCS4 characters, and in that case we need 8 bytes in
the output.

Reviewed-by: Carlos O'Donell <[hidden email]>

> ---
>  iconvdata/ibm1364.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/iconvdata/ibm1364.c b/iconvdata/ibm1364.c
> index b833273aa8..517fe60813 100644
> --- a/iconvdata/ibm1364.c
> +++ b/iconvdata/ibm1364.c
> @@ -150,6 +150,7 @@ enum
>  #define MIN_NEEDED_INPUT   MIN_NEEDED_FROM
>  #define MAX_NEEDED_INPUT   MAX_NEEDED_FROM
>  #define MIN_NEEDED_OUTPUT MIN_NEEDED_TO
> +#define MAX_NEEDED_OUTPUT MAX_NEEDED_TO
>  #define LOOPFCT FROM_LOOP
>  #define BODY \
>    {      \
> @@ -296,6 +297,7 @@ enum
>  
>  /* Next, define the other direction.  */
>  #define MIN_NEEDED_INPUT MIN_NEEDED_TO
> +#define MAX_NEEDED_INPUT   MAX_NEEDED_TO
>  #define MIN_NEEDED_OUTPUT MIN_NEEDED_FROM
>  #define MAX_NEEDED_OUTPUT MAX_NEEDED_FROM
>  #define LOOPFCT TO_LOOP
>