Opened 15 years ago

Closed 15 years ago

Last modified 20 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 15 years ago.
vsnprintf diff

Download all attachments as: .zip

Change History (7)

by SF/khalek, 15 years ago

Attachment: vsnprintf.diff added

vsnprintf diff

comment:1 by fingolfin, 15 years ago

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 by SF/khalek, 15 years ago

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 by fingolfin, 15 years ago

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 by SF/khalek, 15 years ago

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 by SF/khalek, 15 years ago

Owner: set to SF/khalek
Status: newclosed

comment:6 by digitall, 20 months ago

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