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

[Rich Felker]

On Tue, Mar 06, 2007 at 05:22:36PM +0100, Reimar Döffinger wrote:
> 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.

I understand this sentiment, and that there have been bugs (or at
least unintended off-by-one type behavior changes) in some of the
patches. However I'm fairly unhappy with how this whole thread has
been handled. Auditing insecure string behavior is a very difficult
task, and when someone steps up and does it, rather than being
thankful, everyone gets defensive and starts dwelling on minor bugs
and complaining that changes are not needed.

To those complaining, let me say: If you're willing and capable of
doing a complete audit of MPlayer, then by all means let the strncpy
and sprintf crap remain in place and do it. Otherwise, removing the
use of these bad functions is quite a useful service to the project,
in that it makes auditing easier.

Yes it's reasonable to ask Nicholas to fix actual bugs in the patches,
like sizeof(len). But just being critical without offering productive
comments is only going to lead to these patches being dropped and
forgotten, and to a less-robust, less-maintainable MPlayer in the long
term. So I hope we can get whatever issues are really problematic
resolved as soon as possible and get the patches committed, rather
than arguing over stupid things.

Rich


P.S. It was clear when I first read these, and later from talking to
Nicholas, that he put a lot of work into avoiding cosmetic changes in
the indention in order to meet MPlayer policy. Granted there are still
some other minor cosmetic changes that should be committed separately
like the spacing around the parentheses, but all-in-all I think this
is a well-prepared patch set that just needs some minor corrections
and someone willing to follow through with clear steps towards getting
it committed.