Crash in hb_set_anamorphic_size2 and other uninitialised memory accesses

Archive of historical development discussions
Discussions / Development has moved to GitHub
Forum rules
*******************************
Please be aware we are now using GitHub for issue tracking and feature requests.
- This section of the forum is now closed to new topics.

*******************************
Post Reply
tnmurphy
Posts: 1
Joined: Sat Jan 03, 2015 3:21 pm

Crash in hb_set_anamorphic_size2 and other uninitialised memory accesses

Post by tnmurphy »

I had a problem with a crash in hb_set_anamorphic_size2 in the latest head which was 6679 for me. It happened as encoding began (x264, m4v). I used valgrind to investigate.

It turned out that there is a structure hb_geometry_settings_t *geo which is set up by the caller and passed into this function but not all of its members are set and therefore there are a number of uninitialised memory accesses within the function when these uninitialised fields were used and I believe I had a divide by zero or something like that by bad luck. The first one, for example:

int mod = geo->modulus ? EVEN(geo->modulus) : 2;

I could not work out how these values were supposed to be initialised properly. There was no indication in the calling function of where or how they could be fetched from some sort of configuration setting. I'm afraid the whole thing looks like a big mess. Hence I could only do what might be wrong - I memset the structure to 0. This stops my crash but it doesn't seem right to me:

--- libhb/common.c (revision 6681)
+++ libhb/common.c (working copy)
@@ -3000,8 +3000,10 @@

srcGeo.width = title->geometry.width;
srcGeo.height = title->geometry.height;
- srcGeo.par = title->geometry.par;

+ // Clear dest geometry since some fields are optional.
+ memset(&uiGeo, 0, sizeof(uiGeo));
+
uiGeo.geometry.width = job->width;
uiGeo.geometry.height = job->height;
uiGeo.geometry.par = job->par;
Index: libhb/hb.c
===================================================================
--- libhb/hb.c (revision 6681)
+++ libhb/hb.c (working copy)
@@ -906,7 +906,7 @@
/* Set up some variables to make the math easier to follow. */
int cropped_width = src_geo->width - geo->crop[2] - geo->crop[3];
int cropped_height = src_geo->height - geo->crop[0] - geo->crop[1];
- double storage_aspect = (double)cropped_width / cropped_height;
+ double storage_aspect = (double)cropped_width / (double)cropped_height;
int mod = geo->modulus ? EVEN(geo->modulus) : 2;



(the addition of (double) may be of no importance - I was still working out what was going on when I added that.
User avatar
JohnAStebbins
HandBrake Team
Posts: 5712
Joined: Sat Feb 09, 2008 7:21 pm

Re: Crash in hb_set_anamorphic_size2 and other uninitialised memory accesses

Post by JohnAStebbins »

Thanks! memset is the correct thing to do. The double case is redundant.
Post Reply