News:

The Forum Rules and Guidelines
Our forum has Rules and Guidelines. Please, be kind and read them ;).

r8652 or later crashes when loading pak192.comic

Started by ceeac, February 23, 2019, 10:46:05 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ceeac

Steps to reproduce:

  • Install pak192.comic (r717 nightly) via get_pak.sh
  • Start Simutrans and select pak192.comic in the pak selection dialogue
Backtrace:

Thread 1 "sim" received signal SIGSEGV, Segmentation fault.
0x00005555555d6a55 in create_texture_from_tile (image=0x55556aabc820, ref=0x55556152bb60) at descriptor/ground_desc.cc:269
269 sp2[ (ref_w+3)*(y + image->y) + x] = p;
(gdb) bt
#0  0x00005555555d6a55 in create_texture_from_tile (image=0x55556aabc820, ref=0x55556152bb60) at descriptor/ground_desc.cc:269
#1  0x00005555555d88b6 in ground_desc_t::init_ground_textures (world=0x555556d73e50) at descriptor/ground_desc.cc:814
#2  0x000055555593bd6f in karte_t::karte_t (this=0x555556d73e50) at simworld.cc:2053
#3  0x00005555558f593d in simu_main (argc=4, argv=0x7fffffffdd98) at simmain.cc:1238
#4  0x000055555590a864 in sysmain (argc=4, argv=0x7fffffffdd98) at simsys.cc:1056
#5  0x000055555597f447 in main (argc=4, argv=0x7fffffffdd98) at simsys_s2.cc:787


Bisecting points to r8652 as the commit where the crash first appeared.

DrSuperGood

Fixed in R8693.

Buffer overflow error with pixel values being written beyond the end of the created image data buffer. I added a per pixel value write bounds check as a temporary solution.

Someone who knows what that piece of code is meant to be doing needs to check the logic if it is correct. I suspect the reason this crash was happening is that pak192 has a created image with an x/y offset. Neither pak64 or pak128 have created images with x/y offset, instead they are all the full w/h. The pixel copy code might incorrectly make an assumption that the created image is full w/h with no x/y offset and hence why it overruns the buffer. If the copy logic is correct then one can probably optimize away this bounds check by culling entire rows and columns of pixels that would otherwise overflow the buffer.

It would be nice if someone could elaborate what "put multiple copies into dest image" means. I am guessing this is to create some kind of blending effect? Additionally this use of macro is highly confusing and could possibly be expanded to a loop.

Dwachs

#2
The code tries to compute animated water texture for beach tiles. In order to achieve this, the tile-shaped animated graphics are copied into a full square image. This full square image is passed to the routines that do the blending of clima and shores. The tile-shaped image will be copied 5 times with some offset to fill the complete square. Apparently the parameter ref, which is supposed to be a full size square image, does not match this expectation. It is 30px too short in height. Seems a some rewrite of this routine is due.

Edit: I wonder why the fix works. The routine writes pixels into the run-length encoded image, which however does not meet expectations.
Parsley, sage, rosemary, and maggikraut.

DrSuperGood

QuoteEdit: I wonder why the fix works. The routine writes pixels into the run-length encoded image, which however does not meet expectations.
It "works" because it is respecting buffer bounds. If it actually makes sense what happens or is vaguely correct is another question.

Why must it be copied 5 times? Both images were close in size so I do not see there being enough space to tile the one image 5 times into the other.

Flemmbrav

Would it help to provide the animated climate in the same image format/style as the static one?

Dwachs

Quote from: DrSuperGood on February 23, 2019, 08:15:32 PMWhy must it be copied 5 times? Both images were close in size so I do not see there being enough space to tile the one image 5 times into the other.
Because the tile-shaped image does not cover a full square. Then some pixels would not be animated.
Quote from: Flemmbrav on February 23, 2019, 09:03:15 PMWould it help to provide the animated climate in the same image format/style as the static one?
Yes, it would help, but still the code is buggy.
Parsley, sage, rosemary, and maggikraut.

DrSuperGood

#6
A diagram of the input images and expected output (and their formats such as run length encoded) would be nice, even if the output is an in game screenshot of it working, eg with pak64 or 128. From these I could work on creating more correct logic. It is hard to judge what one is looking at from run length encoded buffers in debug mode.

A possible solution might be to expand the image when duplicating it such that it has unique storage for every pixel and run length encoding is removed. One can then modify it without run length encoding or buffer overflow concerns, possibly using similar logic to currently used. Run length encoding could be applied afterwards to save some memory and memory bandwidth before returning the image. Ideally these operations should go either in the image class, or a utility class for manipulating images so that they could be reused in other parts of code as required.

Dwachs

Tried to correct the calculations in r8709. The code assumes that the parameter ref has full rows, an assumption which is everywhere in the code in ground-desc.cc.
Parsley, sage, rosemary, and maggikraut.