[MPlayer-dev-eng] [PATCH 2/4] String handling audit/cleanup take 2

[Reimar Döffinger]

Hello,
On Tue, Mar 06, 2007 at 10:49:53AM -0500, Rich Felker wrote:
> On Tue, Mar 06, 2007 at 12:21:48PM +0100, Reimar Döffinger wrote:
> > On Fri, Mar 02, 2007 at 05:30:09PM -0500, Nicholas Kain wrote:
> > > -  snprintf(str,127,"%d.%d.%d.%d",num[0],num[1],num[2],num[3]);
> > > +  snprintf(str,sizeof(str),"%d.%d.%d.%d",num[0],num[1],num[2],num[3]);
> > 
> > Wow, I'd say in that file there was a proponent of useless use of
> > snprintf at work.
> > I must say, having an integer becoming >= 10^30 is really likely... We
> > might really risk an overflow once ints become 128bit, with the first
> > 1024-bit CPUs...
> 
> Useless? Why? It does not cost anything and provides documentation
> that the code is safe. Without such documentation, auditing is a
> nightmare.

snprintf(str,sizeof(str) may provide such documentation,
but I can't see the advantage of "snprintf(str, 127," over just sprintf,
at least in this case.
And I don't see it in general if the malloc is one or two lines above.
Also note that snprintf may document that the code does not overflow a
buffer, but it does not guarantee much since the truncation might be
just as dangerous.
I consider using snprintf to avoid thinking about the proper buffer size
a serious misuse.
But even that leaves of course enough code to be improved, and lots of
good parts in these patches, though I don't like having to pick and
check them one by one.

Greetings,
Reimar Döffinger