Re: [AD] Patch for rare buffer overflow of do_uconvert with tiny buffer sizes

["Eric Botcazou" <ebotcazou@xxxxxxxxxx>]

> The following program exposes a buffer overflow in current Allegro:
> [...]

Indeed.

> Running it you get the output "(0)(0)(51)(52)", where the second
> byte has been overwritten because do_uconvert tries to write the NULL
> string terminator, which in U_UNICODE has the length of two bytes. I
> also found the undocumented behaviour of using a negative value for
> size, which IMHO is a bad idea.

I agree.

> The following patch tries to remend the buffer overflow with an
> assertion in debug mode, and using the correct sizes in the other
> parts of the lib. If this patch is accepted, a documentation patch
> will follow, documenting the requisite of size being positive and
> big enough to hold at least the string's NULL terminator.

The patch is OK for mainline, except the two following minor nits:
- a double ASSERT on 'size' is overkill. The first one is certainly
superfluous.
- the patch is not complete (the change against aintern.h is missing).

> Running the example with the patch, an assertion in line 666 is
> raised, which is quite cool.

Was this your motivation for adding the double ASSERT? :-)

--
Eric Botcazou