Opened 14 years ago

Closed 11 years ago

Last modified 11 years ago

#800 closed defect (fixed)

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

Reported by: ferrouswheel Owned by: grass-dev@…
Priority: normal Milestone: 6.4.3
Component: Raster Version: svn-develbranch6
Keywords: sprintf, r.reclass, r.random Cc:
CPU: All Platform: All

Description

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.

Attachments (3)

support.c.patch (1.3 KB ) - added by ferrouswheel 14 years ago.
reclass.c.diff (1.1 KB ) - added by ferrouswheel 14 years ago.
reclass_and_random_fix.diff (2.2 KB ) - added by ferrouswheel 13 years ago.
fixes using G_snprintf instead

Download all attachments as: .zip

Change History (17)

by ferrouswheel, 14 years ago

Attachment: support.c.patch added

comment:1 by ferrouswheel, 14 years ago

Component: defaultRaster
CPU: UnspecifiedAll
Platform: UnspecifiedAll
Version: unspecifiedsvn-develbranch6

in reply to:  description ; comment:2 by glynn, 14 years ago

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.

comment:3 by hamish, 14 years ago

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

comment:4 by ferrouswheel, 14 years ago

Milestone: 6.4.06.5.0
Summary: r.random - buffer overflow on long mapset/map namesr.random and r.reclass - buffer overflow on long mapset/map names

Same issue occurs with r.reclass

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

by ferrouswheel, 14 years ago

Attachment: reclass.c.diff added

comment:5 by ferrouswheel, 14 years ago

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.

in reply to:  2 ; comment:6 by neteler, 14 years ago

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

in reply to:  6 comment:7 by glynn, 14 years ago

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.

by ferrouswheel, 13 years ago

Attachment: reclass_and_random_fix.diff added

fixes using G_snprintf instead

comment:8 by ferrouswheel, 13 years ago

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

comment:9 by cmbarton, 12 years ago

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

in reply to:  9 comment:10 by hamish, 12 years ago

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

in reply to:  5 comment:11 by hamish, 12 years ago

Keywords: sprintf added

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

comment:12 by ferrouswheel, 11 years ago

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!

in reply to:  12 comment:13 by hamish, 11 years ago

Keywords: r.reclass r.random added
Milestone: 6.5.06.4.3
Resolution: fixed
Status: newclosed

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

comment:14 by ferrouswheel, 11 years ago

Thanks!

Note: See TracTickets for help on using tickets.