[GRASS5] Buffer overflow in G_recreate_command()...

I thought it'd be great to add category limiting ability to d.area, but
it seems it's possible to crash the monitor when G_recreate_command() is
called and the command line is > 1024 characters. For instance, I have
a vector here with about 9000 categories, and I want to display the
first five hundred as orange.

GRASS ~> d.area map=mymap fillcolor=orange linecolor=white \
         category=$(seq -s ',' 1 500)

This generates a command line greater than 1024 bytes, leading to a
unterminated charachter buffer which eventually causes the monitor to
crash. (G_recreate_command() uses a static buff[1024] ...).

I wonder if the buffers for G_recreate_command() and the display drivers
(at least for the pad list) should be up to ARG_MAX or at least
_POSIX_ARG_MAX which must be at least 4096. G_recreate_command() should
also have better behavior when it's limits are reached (it currently
abuses strcat).

Any thoughts?

--
Eric G. Miller <egm2@jps.net>

On Sun, Jun 03, 2001 at 05:00:53PM -0700, Eric G. Miller wrote:
[snip]

I wonder if the buffers for G_recreate_command() and the display drivers
(at least for the pad list) should be up to ARG_MAX or at least
_POSIX_ARG_MAX which must be at least 4096. G_recreate_command() should
also have better behavior when it's limits are reached (it currently
abuses strcat).

I modified G_recreate_command to use a buffer of ARG_MAX size and to
make sure the buffer isn't overrun. Still, I get a sigpipe from the X
server and it exits. I traced it on the client side to the point in
Rasterlib where flushout() is called by _send_char. Is there some
problem here if the buffer is flushed, but the communication is not
complete?

--
Eric G. Miller <egm2@jps.net>

"Eric G. Miller" wrote:

I thought it'd be great to add category limiting ability to d.area, but
it seems it's possible to crash the monitor when G_recreate_command() is
called and the command line is > 1024 characters. For instance, I have
a vector here with about 9000 categories, and I want to display the
first five hundred as orange.

GRASS ~> d.area map=mymap fillcolor=orange linecolor=white \
         category=$(seq -s ',' 1 500)

This generates a command line greater than 1024 bytes, leading to a
unterminated charachter buffer which eventually causes the monitor to
crash. (G_recreate_command() uses a static buff[1024] ...).

I wonder if the buffers for G_recreate_command() and the display drivers
(at least for the pad list) should be up to ARG_MAX or at least
_POSIX_ARG_MAX which must be at least 4096. G_recreate_command() should
also have better behavior when it's limits are reached (it currently
abuses strcat).

Any thoughts?

BTW: In grass51 d.vect has cat= option which accepts strings like:
cat=1,28,156-217,512 or cat=1-500 for you
and it is based on new function
Vect_str_to_cat_list (char *str, struct cat_list *list)

Radim

Eric G. Miller wrote:

> I wonder if the buffers for G_recreate_command() and the display drivers
> (at least for the pad list) should be up to ARG_MAX or at least
> _POSIX_ARG_MAX which must be at least 4096. G_recreate_command() should
> also have better behavior when it's limits are reached (it currently
> abuses strcat).

I modified G_recreate_command to use a buffer of ARG_MAX size and to
make sure the buffer isn't overrun. Still, I get a sigpipe from the X
server and it exits. I traced it on the client side to the point in
Rasterlib where flushout() is called by _send_char. Is there some
problem here if the buffer is flushed, but the communication is not
complete?

Note that the driver reads the second argument for PAD_APPEND_ITEM
(and most other commands which take an arbitrary string as an
argument) into a 1024 byte buffer.

This limitation is simply a result of process_command() using a
fixed-size buffer for reading strings from the client; there isn't any
limitation elsewhere.

Any suggestions as to how big this buffer ought to be? Or does it need
to be dynamically allocated?

--
Glynn Clements <glynn.clements@virgin.net>

On Mon, Jun 04, 2001 at 07:57:25AM +0100, Glynn Clements wrote:

Note that the driver reads the second argument for PAD_APPEND_ITEM
(and most other commands which take an arbitrary string as an
argument) into a 1024 byte buffer.

This limitation is simply a result of process_command() using a
fixed-size buffer for reading strings from the client; there isn't any
limitation elsewhere.

Any suggestions as to how big this buffer ought to be? Or does it need
to be dynamically allocated?

I don't know that the buffer needs to be bigger, but maybe. With the
PAD_LIST, it appears the data eventually gets put in dynamic memory
anyway. One possibility would be to add a "SIZE" argument to the text
commands so the read commands would know exactly how much space to
allocate and if the "transaction" was complete. Other possibility is to
make the buffer "ARG_MAX" + 1 size, though that may be a little greedy
(around 13.5k here...). The second is the easiest to implement and will
guarantee any command possibly given for that system will fit into the
read buffer.

--
Eric G. Miller <egm2@jps.net>

Eric G. Miller wrote:

On Mon, Jun 04, 2001 at 07:57:25AM +0100, Glynn Clements wrote:
> Note that the driver reads the second argument for PAD_APPEND_ITEM
> (and most other commands which take an arbitrary string as an
> argument) into a 1024 byte buffer.
>
> This limitation is simply a result of process_command() using a
> fixed-size buffer for reading strings from the client; there isn't any
> limitation elsewhere.
>
> Any suggestions as to how big this buffer ought to be? Or does it need
> to be dynamically allocated?

I don't know that the buffer needs to be bigger, but maybe. With the
PAD_LIST, it appears the data eventually gets put in dynamic memory
anyway. One possibility would be to add a "SIZE" argument to the text
commands so the read commands would know exactly how much space to
allocate and if the "transaction" was complete. Other possibility is to
make the buffer "ARG_MAX" + 1 size, though that may be a little greedy
(around 13.5k here...).

ARG_MAX is 128kb on Linux 2.4; even greedier. Stevens implies that
sysconf(_SC_ARG_MAX) is 1Mb on SunOS 4.1.1, although it doesn't say
whether the ARG_MAX macro is the same. Ultimately I don't think that
anything actually imposes an upper limit on what ARG_MAX might be.

As RECTEXT() is only ever used on two variables ("name" and "text"),
I'll change it to dynamically (re)allocate the memory.

--
Glynn Clements <glynn.clements@virgin.net>

Glynn Clements wrote:

> I don't know that the buffer needs to be bigger, but maybe. With the
> PAD_LIST, it appears the data eventually gets put in dynamic memory
> anyway. One possibility would be to add a "SIZE" argument to the text
> commands so the read commands would know exactly how much space to
> allocate and if the "transaction" was complete. Other possibility is to
> make the buffer "ARG_MAX" + 1 size, though that may be a little greedy
> (around 13.5k here...).

ARG_MAX is 128kb on Linux 2.4; even greedier. Stevens implies that
sysconf(_SC_ARG_MAX) is 1Mb on SunOS 4.1.1, although it doesn't say
whether the ARG_MAX macro is the same. Ultimately I don't think that
anything actually imposes an upper limit on what ARG_MAX might be.

As RECTEXT() is only ever used on two variables ("name" and "text"),
I'll change it to dynamically (re)allocate the memory.

OK; this is done. There shouldn't be any limitation now.

--
Glynn Clements <glynn.clements@virgin.net>