[vlc-devel] [vlc-commits] modules: Adjust integer limits

[Rémi Denis-Courmont]

Le perjantaina 14. lokakuuta 2016, 18.32.55 EEST Hugo Beauzée-Luyssen a 
écrit :
> On 10/14/2016 06:26 PM, Rémi Denis-Courmont wrote:
> > 	Hello,
> > 
> > Le perjantaina 14. lokakuuta 2016, 18.22.00 EEST Hugo Beauzée-Luyssen a
> > 
> > écrit :
> >> vlc | branch: master | Hugo Beauzée-Luyssen <hugo at beauzee.fr> | Fri Oct
> >> 14
> >> 18:21:08 2016 +0200| [e908026c45d91a18b5a3835953edd47a822b4a40] |
> >> committer: Hugo Beauzée-Luyssen
> >> 
> >> modules: Adjust integer limits
> >> 
> >>> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=e908026c45d91a18b
> >>> 5a
> >>> 3835953edd47a822b4a40
> >> 
> >> ---
> >> 
> >>  src/modules/entry.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/src/modules/entry.c b/src/modules/entry.c
> >> index ae8e041..7698499 100644
> >> --- a/src/modules/entry.c
> >> +++ b/src/modules/entry.c
> >> @@ -129,8 +129,8 @@ static module_config_t *vlc_config_create (module_t
> >> *module, int type) memset (tab + confsize, 0, sizeof (tab[confsize]));
> >> 
> >>      if (IsConfigIntegerType (type))
> >>      {
> >> 
> >> -        tab[confsize].max.i = INT_MAX;
> >> -        tab[confsize].min.i = INT_MIN;
> >> +        tab[confsize].max.i = INT64_MAX;
> >> +        tab[confsize].min.i = INT64_MIN;
> > 
> > When 64-bits was added, we kept the INT boundaries by default.
> > In many cases we have legacy code along the lines of:
> > 
> > int a = var_InheritInteger(obj, "varname");
> > 
> > That can result in arithmetic overflow with your patch.
> > 
> > Maybe we should have helper macros to set the range (e.g. add_int64,
> > add_uint32).
> > 
> >>      }
> >>      else if( IsConfigFloatType (type))
> >>      {
> >> 
> >> _______________________________________________
> >> vlc-commits mailing list
> >> vlc-commits at videolan.org
> >> https://mailman.videolan.org/listinfo/vlc-commits
> 
> Hi,
> 
> That's a fair point. Although int a = var_InheritInteger should and will
> trigger a compiler warning, I'm afraid fixing all call sites could be
> long and painfull, if not error prone...

I don´t really agree that the compiler should issue a warning. Converting an 
integer value to a narrower signed type is a promise to the compiler that the 
value is within the range of the narrower type. It happens all the time in C, 
and is essentially unavoidable in case of implicit integer promotion (e.g. 
variable arguments).

There exists profiles of C that forbid it. But there is also ample anecdotal 
evidence within the VLC code base that, by default, compilers do not warn in 
these circumstances.

> I have no strong opinion here, beside the fact that without this commit
> (or with Filip recent commit, enforcing the check), any int64 passed
> through command line will be clamped in between the wrong values.

It was debatable whether plugins should have relied on the value range, or 
have checked/enforced it explicitly. But I would argue that a plugin that 
relied actually on out of range values was and is just plain broken.

The default range is INT_MIN..INT_MAX, has always been, and is more than 
enough in most use cases. If you need a larger range, set it correctly where 
appropriate.

I agree that using INT64_MIN..INT64_MAX as default would be cleaner. But 
until/unless code paths are audited and fixed properly, we shan´t venture 
there IMO.

-- 
Rémi Denis-Courmont
Nonsponsored VLC developer
http://www.remlab.net/CV.pdf