[FIXED] Glitch/Bug between _FREEIMAGE/_COPYIMAGE/_MEM (Critical Error #308)

[RhoSigma]

Hi QB64 Team,

It seems I found a longstanding glitch/bug between _FREEIMAGE, _COPYIMAGE and the _MEM functions (probably _MEMIMAGE), which leads to an Critical Error #308 (memory has been freed).

The error happens, whenever _COPYIMAGE does use a handle, which was already used by another image earlier but which got freed in the meantime and hence is available for re-use. I could verify this error starting from the old SDL 0.954 version through different GL builts in between up to the latest GL development built available from QB64.org.

I've made a small program to show the issue. Note you need adjust the _LOADIMAGE line (3) to use any image on your system (must be at least 1024x768).

To get an understanding of the workflow, first read the comments right above the ModifyBrightness&() function (lines 37-40), then the comments in the main module (lines 14-26).

If you run the program as is, it will show the loaded image and wait for a keypress. When you press a key, it will do the _FREIMAGE and then the final ModifyBrightness&() call and will error out with #308 immediately. After that try to comment the _FREEIMAGE line !!OR!! commenting the _COPYIMAGE in ModifyBrightness&() and instead uncommenting the _NEWIMAGE/_PUTIMAGE pair (lines 63-67) and try again. It will work in both cases, a further keypress will then end the program.

Code: QB64: [Select]

1. '--- 1st show the original image ---
2. _TITLE "The original Image, press any key..."
3. ihan& = _LOADIMAGE("AnyImage.jpg", 32) 'any image file (at least 1024x768)
4. IF ihan& >= -1 THEN ERROR 53 'file not found
5. SCREEN ihan&
6.  
7. '--- now make a darken cascade ---
8. _TITLE "Darken Cascade, press any key..."
9. dark& = ModifyBrightness&(ihan&, -0.15, 250, 70, 820, -1)
10. darker& = ModifyBrightness&(dark&, -0.15, 275, 95, 795, -1)
11. COLOR _RGB32(255, 255, 200): PRINT "Used Handle: "; dark&: SLEEP
12.  
13. '--------------------------------------------------------------------
14. '--- With activated _FREEIMAGE the freed handle is re-used by the
15. '--- next _COPYIMAGE call done in ModifyBrightness&() called right
16. '--- below the _FREEIMAGE, but the handle in question obviously gets
17. '--- not correctly initialized by _COPYIMAGE, as the _MEM function(s)
18. '--- used later to iterate through the image do fail to work on it
19. '--- (Critical Error #308 (memory was freed)).
20. '-----
21. '--- If otherwise the _FREEIMAGE call is commented out, then it works
22. '--- with _COPYIMAGE, as a new clean handle must be used instead.
23. '-----
24. '--- In alternative it also works with activated _FREEIMAGE, but change
25. '--- the _COPYIMAGE in FUNCTION ModifyBrightness&() into a combination
26. '--- of _NEWIMAGE and _PUTIMAGE instead to make the image copy.
27. '--------------------------------------------------------------------
28. _FREEIMAGE dark&
29. darkest& = ModifyBrightness&(darker&, -0.15, 300, 120, 770, -1)
30. '--------------------------------------------------------------------
31.  
32. SCREEN darkest&
33. COLOR _RGB32(255, 255, 200): PRINT "Used Handle: "; darkest&: SLEEP
34. '--- done ---
35. SYSTEM
36.  
37. 'This function is a cutout from 'QB64Library\IMG-Support\imageprocess.bm',
38. 'which is part of my Libraries Collection. It takes a source image handle
39. 'and returns a modified image in a new handle usually made with _COPYIMAGE.
40. 'This seems to fail, if for the copy an old (freed) handle is re-used.
41. '---------------------------------------------------------------------
42. 'NAME
43. '   ModifyBrightness -- Brightness adjustment, keep alpha
44. '
45. 'TEMPLATE
46. '   newImg& = ModifyBrightness& (shan&, change#, minX%, minY%, maxX%, maxY%)
47. '
48. '   change# -- must be between -1.0 and 1.0  (clipped)
49. '
50. ' DESCRIPTION
51. '   This changes the general brightness of the image. The smaller the
52. '   value is the darker the image becomes.  0.0 means no change.
53. '   The alpha channel of the source image is retained and copied 1:1.
54. '---------------------------------------------------------------------
55. FUNCTION ModifyBrightness& (shan&, change#, minX%, minY%, maxX%, maxY%)
56. ModifyBrightness& = -1 'so far return invalid handle
57. IF shan& < -1 THEN
58.     IF _PIXELSIZE(shan&) = 4 THEN
59.         '--- get source image size and a copy of the image ---
60.         swid% = _WIDTH(shan&): shei% = _HEIGHT(shan&)
61.  
62.         '--------------------------------------------------
63.         '--- This works only, if a new handle must be used,
64.         nhan& = _COPYIMAGE(shan&)
65.         '--- and this works for both re-used and new handles
66.         'nhan& = _NEWIMAGE(swid%, shei%, 32)
67.         '_PUTIMAGE ,shan&, nhan&
68.         '--------------------------------------------------
69.  
70.         '--- check selected processing area ---
71.         IF minX% < 0 OR minX% >= swid% THEN minX% = 0
72.         IF maxX% < 0 OR maxX% >= swid% THEN maxX% = swid% - 1
73.         IF minY% < 0 OR minY% >= shei% THEN minY% = 0
74.         IF maxY% < 0 OR maxY% >= shei% THEN maxY% = shei% - 1
75.         '--- process copied image ---
76.         IF nhan& < -1 THEN
77.             '--- build histogram transformation table ---
78.             IF change# < -1.0# THEN change# = -1.0
79.             IF change# > 1.0# THEN change# = 1.0
80.             REDIM hist%(255)
81.             FOR i% = 0 TO 255
82.                 v% = FIX(i% * (change# + 1.0#))
83.                 IF v% < 0 THEN v% = 0: ELSE IF v% > 255 THEN v% = 255
84.                 hist%(i%) = v%
85.             NEXT i%
86.             '--- for speed we do direct memory access ---
87.             DIM nbuf AS _MEM: nbuf = _MEMIMAGE(nhan&)
88.             '--- iterate through pixels ---
89.             FOR y% = minY% TO maxY%
90.                 noff%& = nbuf.OFFSET + (y% * swid% * 4) + (minX% * 4)
91.                 FOR x% = minX% TO maxX%
92.                     '--- get pixel ARGB value from source ---
93.                     _MEMGET nbuf, noff%&, orgb~&
94.                     '--- modify pixel components ---
95.                     newA% = _ALPHA32(orgb~&) 'ignored (put through)
96.                     newR% = hist%(_RED32(orgb~&))
97.                     newG% = hist%(_GREEN32(orgb~&))
98.                     newB% = hist%(_BLUE32(orgb~&))
99.                     '--- put new pixel ARGB value to dest ---
100.                     nrgb~& = _RGBA32(newR%, newG%, newB%, newA%)
101.                     _MEMPUT nbuf, noff%&, nrgb~&
102.                     '--- set next pixel offset ---
103.                     noff%& = noff%& + 4
104.                 NEXT x%
105.             NEXT y%
106.             '--- cleanup ---
107.             _MEMFREE nbuf
108.             ERASE hist%
109.             '--- set result ---
110.             ModifyBrightness& = nhan&
111.         END IF
112.     END IF
113. END IF
114. END FUNCTION
115.  
116.  

[FellippeHeitor]

Thanks for the report, RhoSigma. It will be investigated.

[SMcNeill]

A simpler demo of the issue, as reported:

Code: QB64: [Select]

1. DIM m AS _MEM
2.  
3. ihan& = _LOADIMAGE("BeachGirl.jpg", 32) 'any image file (at least 1024x768)
4. IF ihan& >= -1 THEN ERROR 53: END 'file not found
5. SCREEN ihan&
6.  
7. dark& = _COPYIMAGE(ihan&) 'make a copy
8. m = _MEMIMAGE(dark&)
9. PRINT _MEMGET(m, m.OFFSET, _UNSIGNED _BYTE) 'just print the first byte of info to show we can access memory from the image
10. _MEMFREE m
11.  
12.  
13. darker& = _COPYIMAGE(dark&) 'make a copy of the copy
14.  
15. _FREEIMAGE dark& 'free the first copy
16.  
17. m = _MEMIMAGE(darker&)
18. PRINT _MEMGET(m, m.OFFSET, _UNSIGNED _BYTE)
19. _MEMFREE m
20.  
21.  
22. END

[RhoSigma]

Steve, i'd say you even show a different issue here, as you first make a 2nd copy (darker&) then free the 1st one and finally (without making a 3rd copy which would re-use the handle of the just freed 1st copy) you do a memory access to the 2nd copy, which then fails.

So for me it looks like the _FREEIMAGE dark& will even corrupt the previously copied darker& handle, so that the following memory access made on darker& will fail.

That's definitly another bug, i was only aware of it for re-used handles, but your example shows that _FREEIMAGE does obviously corrupt innocent handles, which were not even specified to free.

[SMcNeill]

Doing a little tracing of this error reveals the following for me:

In libqb.cpp, we can trace the glitch to the last line in sub__freeimage:

 

Code: QB64: [Select]

1. freeimg(i);

Tracing back into the freeimg routine in libqb.cpp, we can narrow the issue down to the IF statement which deals with mem_lock status:

Code: QB64: [Select]

1.   IF (img[i].lock_id){
2.     free_mem_lock((mem_lock*)img[i].lock_offset);//untag
3.   }

Remark out that free_mem_lock statement and the program will run without generating the error message -- and run with no issues.


I'm thinking the glitch is in the .lock_offset, as it seems as if _COPYIMAGE doesn't create a new lock_offset, but instead just basically copies the majority of that information from the original image to the copied version. 

image ihan& gets the original .lock_offset, then image dark& basically copies that value, and then image &darker basically copies that same exact value...  Freeing any of those makes our _MEM routines fail.  (if ( ((mem_lock*)(((mem_block*)(blk))->lock_offset))->id!=((mem_block*)(blk))->lock_id ){error(308); goto fail;})

The solution is still beyond me, but it appears that lock_offset may be the issue at the root of the problem. 

The memory is good.  The memblocks are valid -- just remark out that call to free_mem_lock in the freeing routine, and you'll see that we can read and write to the memory properly.  The images aren't corrupt.  _MEM isn't corrupt.  It's just a glitch with that lock_offset being copied and freed once, which seems to be the issue to me. 

[FellippeHeitor]

Hopefully it's something simple.

If you do decide to push changes, please make sure to work on the development branch.

[RhoSigma]

Hi QB64 team,
I've found the easiest fix for this problem.

As far I've inspected libqb.cpp, new images are always created without lock_id/lock_offset. Just in the moment when I'll use the image with _MEM functions, then a lock is assigned to this image by the _MEMIMAGE call. Also _MEMIMAGE is smart enough to check for an existing lock first, before creating a new one, hence we can call _MEMIMAGE multiple times on the very same image and it will always use the first assigned lock.

The _FREEIMAGE function makes sure to kill an existing lock on the freed image (if any), if the image was never used with _MEM functions, then there's even no lock to kill.

As already explained by Steve, _COPYIMAGE just makes a copy of the general data of the given image, hence it also duplicates an existing lock, but the lock was intended for the original image only and causes us problems when one of the images (copy or original) is freed.

Now we know _MEMIMAGE is a smart function when it comes to lock the image, so all we need to do in _COPYIMAGE is to make the copy of course, but right after that we clear out the lock related fields in the new image structure to force _MEMIMAGE to create a new lock for the copied image. We otherwise don't touch an existing lock of the original image, as this must stay in effect.

So finally after all the theory, we just need to add one line of code in file libqb.cpp in the func_copyimage its "//duplicate structure" block right after the memcpy call.

Code: C: [Select]

1.     ...
2.     ...
3.     ...
4.     //duplicate structure
5.     i2=newimg();
6.     d=&img[i2];
7.     memcpy(d,s,sizeof(img_struct));
8.     img[i2].lock_id=NULL; img[i2].lock_offset=NULL; // force _MEMIMAGE to get a new lock for the copy
9.     ...
10.     ...
11.     ...
12.  

I've tested this fix in various QB64 versions inclusive my oldest (v0.954 SDL) and the latest available development build and it works, so somebody can please make the change in the repository.

[FellippeHeitor]

A one-liner fix is always a very satisfying solution. Thanks for investigating, RhoSigma.

You sure you don't wanna make the change yourself? It'd be a pleasure to welcome a patch from you to the repo (I can walk you through if you need me to).

[RhoSigma]

...You sure you don't wanna make the change yourself?...

:), yes I'am, please go on do it for me, see the text beneth my avatar? I hardly can find time to improve on my GuiTools Framework, so I won't start going any deeper into QB64 development at this point. This fix in an exception, as the glitch came up while working on GuiTools.

BTW -  The problem is not really the time, but my wife. Every day I spend more than 1-2 houres in front of the pc I get into trouble (should have known this 20 years ago) :(

[FellippeHeitor]

This is not going any deeper than you've gone already. You've got it done, it'd be a matter of copy pasting the line.

It'd take you five minutes. I'll let you consider. We have time. I'll post instructions, may encourage others.

[RhoSigma]

Five minutes? - Well, then post the instructions and I'll see...

[FellippeHeitor]

First, make sure you made your edit to the latest version of libqb.cpp (this one: https://raw.githubusercontent.com/Galleondragon/qb64/development/internal/c/libqb.cpp)

• Create a free account at www.github.com
• Go to https://github.com/Galleondragon/qb64 and click the Fork button
• Now you have your own repository fork at your-account/qb64, which you can edit. Where you see Branch: master, click to change to Branch: development
• Navigate to internal/c
• Click on Upload files and upload your edited version of libqb.cpp

After all is done, GitHub will offer you a button to Make a pull request, which you will do. Then you're done.

You became a contributor. The rest is with us.

[RhoSigma]

Done, hopefully everything went well, GitHub is a monster - seems much to complex for my taste.

Username "RhoSigma" was already taken, so you find me as "RhoSigma-QB64" instead.

[FellippeHeitor]

Great! I’ve just merged your pull request into the development branch and the dev build at www.qb64.org should be updated in a few minutes with this patch.

Edit: development build updated and available.

[RhoSigma]

Well, now I've one remaining question, does merging branches automatically keep the fork in my repository up-to-date, or must I get a new fork from galleondragon/qb64 whenever I've to contribute something?