Opened 14 years ago

Closed 14 years ago

Last modified 10 months ago

#8409 closed patch

replace vsprintf usage with vsnprintf

Reported by: SF/khalek Owned by: SF/khalek
Priority: normal Component: --Other--
Keywords: Cc:
Game:

Description

vsprintf is not a safe string function, vsnprintf
should be used.

The only problem is the apparent lack of implementation on
palmos and perhaps other platforms?

Local implementations should be created for these
platforms.

Ticket imported from: #1168900. Ticket imported from: patches/514.

Attachments (1)

vsnprintf.diff (6.7 KB) - added by SF/khalek 14 years ago.
vsnprintf diff

Download all attachments as: .zip

Change History (7)

Changed 14 years ago by SF/khalek

Attachment: vsnprintf.diff added

vsnprintf diff

comment:1 Changed 14 years ago by fingolfin

Indeed, vsnprintf is not very portable, which is the main reason I didn't use
it so far (I was considering switching to it at some point).

Do we really want to provide our own vsnprintf implementation (i.e. the full
code for a full blown printf clone), just so we can avoid some hypothetical
overflows in a non-security relevant area? hmmm

comment:2 Changed 14 years ago by SF/khalek

People are likely to look at the code and use it or
something like it in a security sensitive area. Teaching
people good programming practice is important...

A local implementation would only be used if there was not
an existing
implementation, the palmos backend already does this for a
lot of things
because Palm only seem to implement most common functions in
a version
of PalmOS that isn't actually available on real hardware yet.

comment:3 Changed 14 years ago by fingolfin

I think that justification is nonsense -- people who look at
our code and thing it is an example of how to code for a
security sensitive area are nuts anyway. E.g. there are
zillions of places where we use snprintf. Or read from a
file and work with its content w/o verifying it properly.
Etc. etc. -- if you install ScummVM in a way that gives it
too many rights, you are doomed either way.

Anyway, apparently you just checked this patch in anyway, so
I assume it can be closed. If we get troubles with porters,
we can still revert it.

comment:4 Changed 14 years ago by SF/khalek

Given we were already using vsprintf I don't think asking
for vsnprintf is that
much of a stretch.

Peope get code examples from all over the place and
sometimes things
as non critical as ScummVM could become critical. It would
be nice to
be able to point people at the code as an example of good
programming
practice. I am of the opinion it is necessary to not
dismiss things like
using safe string functions because a context is assumed for
the code.

Once every few weeks someone comes along wanting to adapt
ScummVM for some other purpose. The use of these bad string
functions can hopefully be limited by having fewer examples
of their use.

I suspect you mean sprintf not snprintf. There are many
places where
strcat and strcpy are used instead of safe things like
strlcat and strlcpy in
the code as it stands now as well.

comment:5 Changed 14 years ago by SF/khalek

Owner: set to SF/khalek
Status: newclosed

comment:6 Changed 10 months ago by digitall

Component: --Other--
Note: See TracTickets for help on using tickets.