GNOME Bugzilla – Bug 759840
orcprogram-c: Varnames bounds seem broken
Last modified: 2018-01-15 10:32:04 UTC
The following commit seems broken as the variables arrays are ORC_N_COMPILER_VARIABLES but the varnames arrays are smaller. This might break something. Of course the old code was also broken probably, or at least misleading. But this change might introduce regressions because fewer variables are possible. commit 951091788b8496868bf86b2f853f5214659e782a Author: Luis de Bethencourt <luisbg@osg.samsung.com> Date: Mon Dec 14 13:46:52 2015 +0000 orcprogram-c: avoid running out of bounds of varnames These two for loops use varnames[i], since varname has 48 items avoid running out of bounds by running the for loop up to that limit. CID 1147004
Luis, any news here?
I think we should revert to be on the safe side for now so we can make a release. Then we can re-visit this later.
After considering this a bit more and reading the code again I agree with Tim. commit 45ef23c09a7147039b9a1109c26ad431228e3114 Author: Luis de Bethencourt <luisbg@osg.samsung.com> Date: Wed Feb 17 15:05:49 2016 +0000 Revert "orcprogram-c: avoid running out of bounds of varnames" This reverts commit 951091788b8496868bf86b2f853f5214659e782a. This isn't a proper fix and the risk of bringing in a regression is too high for the small and incomplete improvement.
Created attachment 321503 [details] [review] avoid reading varnames[] out of bounds Protect towards the unlikely (yet still possible) out of bounds reading of varnames[] when get_varname_stride() gets called from a var higher than 48.
Comment on attachment 321503 [details] [review] avoid reading varnames[] out of bounds And does this work then for >= 48? Probably not, in which case an assertion might be nicer for now?
Switched to using an assertion. Thanks for the feedback :) commit fe2ac0c1d066a37809e2169a9f591dbc8f09707f Author: Luis de Bethencourt <luisbg@osg.samsung.com> Date: Mon Dec 14 16:20:18 2015 +0000 orcprogram-c: check array bound in get_varname_stride() https://bugzilla.gnome.org/show_bug.cgi?id=759840
orc is coverity-clean now. Closing.