[FFmpeg-devel] [PATCH 2/3] avcodec/utils: Fix several integer overflows.

[Nicolas George]

Le sextidi 16 prairial, an CCXXV, Ronald S. Bultje a écrit :
> This reminds me of g_return_val_if_fail in glib, and in some situations
> it's quite sensible and helpful.

I think so too.

But g_return_val_if_fail() has a flaw: it prints a warning and returns,
at least with default builds of the library. A lot of applications
developers ignore these warnings when they have no obvious adverse
drawbacks. A crash is better IMHO.

Note that I am only advocating a crash for invalid uses of the API that
are obvious and easy to check, like:

	* @param foo  pointer to return value, must not be NULL

because it is easy for the application to wrap it and make the test:

	int frobnicate_or_bail(type *foo) {
	    if (foo == NULL) {
		warn("I'm afraid I can't do that");
		return ERROR_INVALID;
	    }
	    return frobnicate(foo);
	}

My main argument is that it is almost always unnecessary: when the
argument to a function must not be NULL, it is usually because the
natural way of calling the function yields a non-NULL pointer. For
example, in this particular case, the natural way of calling the
function is probably "ret = frobnicate(&value);".

The same goes for seek: when you call "seek(fd, pos, SEEK_SET);", you do
not need to check that SEEK_SET is a valid value: it is. I would even
argue that fd should get the same treatment, since an application doing
things on a fd without knowing if it is valid is broken and may
overwrite precious files (crashing will prevent that). On the other
hand, the application cannot easily check if fd is seekable or if pos is
valid: these conditions must cause an error return code, not a crash.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170605/73950a9d/attachment.sig>