r.random and r.reclass - buffer overflow on long mapset/map names

[ferrouswheel]

In r.random/support.c there are sprintf calls which cause buffer overflow errors when the map names and mapsets are too long. I've attached a patch to replace with snprintf. This truncates the messages, but the RECORD_LEN for the History struct is by default only 80.

[glynn]

Replying to ferrouswheel:

In r.random/support.c there are sprintf calls which cause buffer overflow errors when the map names and mapsets are too long. I've attached a patch to replace with snprintf.

snprintf() isn't in C89; if you want to use it, you need to add a configure check, and provide an alternate in case it isn't available.

[hamish]

Hi,

title overflow fixed in 6.5svn with r39679. I notice that a similar problem exists with the (priorly redundant but now relevant) data source metadata. IIRC those are limited to 80 (??) chars currently by gis.h.

the @mapset part can be dropped for starters, and G_command_history() added.

nothing ported to other branches yet.

Hamish

[ferrouswheel]

Same issue occurs with r.reclass

I've attached an alternative way resolve it which doesn't use snprintf, but is rather hacky.

[ferrouswheel]

I wonder if there shouldn't be a wrapper method for updating the History object.

Doing a grep for the offending line:

grep -r sprintf\(hist.dat *

Shows the following modules making unsafe assumptions about map name length: r.buffer, r.carve, r.random, r.recode, r.slope.aspect, r.sunmask, and the simwe modules.

[neteler]

Replying to glynn:

Replying to ferrouswheel:

In r.random/support.c there are sprintf calls which cause buffer overflow errors when the map names and mapsets are too long. I've attached a patch to replace with snprintf.

snprintf() isn't in C89; if you want to use it, you need to add a configure check, and provide an alternate in case it isn't available.

We do have G_snprintf() in lib/gis/snprintf.c which is a "private" implementation. Should that be used instead?

Markus

[glynn]

Replying to neteler:

snprintf() isn't in C89; if you want to use it, you need to add a configure check, and provide an alternate in case it isn't available.

We do have G_snprintf() in lib/gis/snprintf.c which is a "private" implementation. Should that be used instead?

Yes. Although vsnprintf() isn't guaranteed to exist, so far no-one has complained about GRASS failing to build, which suggests that platforms lacking vsnprintf() are uncommon.

[ferrouswheel]

fixes using G_snprintf instead

[ferrouswheel]

Any chance of my latest simple patch getting applied? It would be most appreciated - thanks!

[cmbarton]

Just discovered that this also affects r.recode. It crashes with a very confusing buffer overflow error if the file names are too long.

Michael

[hamish]

Replying to cmbarton:

Just discovered that this also affects r.recode. It crashes with a very confusing buffer overflow error if the file names are too long.

probably these two:

recode.c:    sprintf(hist.edhist[0], "recode of raster map %s", name);
recode.c:    sprintf(hist.datsrc_1, "raster map %s", name);

which has a max RECORD_LEN of 80 in grass 6.

Hamish

[hamish]

Replying to ferrouswheel:

Doing a grep for the offending line: grep -r sprintf\(hist.dat * Shows the following modules making unsafe assumptions about map name length: r.buffer, r.carve, r.random, r.recode, r.slope.aspect, r.sunmask, and the simwe modules.

all of these now updated in devbr6. please test. (grass7 is not affected as the 80 column restriction on the history components has been removed)

Hamish

[ferrouswheel]

In grass64_release, the r.reclass issue is fixed by user "mmetz". But it is not fixed for r.random?

It's been 3 years and it's sad to see the ubuntu packages still contain these simple buffer overflow bugs!

[hamish]

Replying to ferrouswheel:

In grass64_release, the r.reclass issue is fixed by user "mmetz". But it is not fixed for r.random?

I fixed in 19 months ago (r48460); the r.reclass half of r48460 got backported, but the r.random half didn't. Now done in r55628. Trunk was fixed by Glynn a long time ago during the revamp of the hist/ structure.

(see also backport needed(?) for #1082: r.random cover= gets stray data in first and last rows)

Hamish

[ferrouswheel]

Thanks!