Some hardening, but also I couldnt resist a bit of refactor ;D
Tightens malformed-pak handling so the loader fails with a
diagnostic instead of walking past the buffer.
- obj_desc_t stores nchildren; get_child<T>(i) dbg->fatals on
out-of-bounds i. Pins a NULL-deref reachable via
skin_reader_t::register_obj on a 0-child MENU node.
- node_body wraps one node's fread'd bytes in a bounds-checked
cursor that carries the reader's type name. Each reader's
read_node starts with
auto p = node_body(fp, node.size, get_type_name());
if (!p) return NULL;
and the existing decode_*(p) calls keep working via free
decode_uint*(node_body&) overloads. The raw-pointer
decode_uint*(char*&) overloads stay; pakset_manager.cc still
uses them for the 5 node-header reads. Fatal on overrun
reports offset / total / need / have and the reader's name.
- read_node_info caps a large-record node.size at remaining
file bytes so a truncated pak doesn't drive a multi-GB
malloc. obj_named_desc_t::get_name picks up the same
NULL-check the sibling get_copyright already has;
register_desc<> skips when get_name returns NULL.
One gap not closed here: image_t::alloc(decode_uint32(p)) still
takes its length straight from the buffer. Same
allocation-size class as the read_node_info cap but at the
reader level. Happy to follow up if useful.
Did you check of how much this additionally fseek changes the loading of paks? Because this is done many million times.
Also, simutrans would crash on a malformed pak later anyway, as there is no system in place to unload paks.
Also, the nchildren entry adds 8 bytes to every node in memory (for 64 bit builds), which is a big memory penalty cachewise for something on load time. One may discuss this for debug builds though.
Objs without names cannot exist since this is the fundamental mechanism as how simutrans refers to objects. I have added code to makeobj to explicitely fail on building instead of crashing.
Personally, auto variables are evil. In C you should always know your variables. Also, your node_body leaks memory as the char is never deleted. The array_tpl is allocated on the stack and hence freed afterwards.
I am not opposed to changing decode_xxx to do bounds check (although it still will fail to fatal without a chance to output the faulty pack name). However, that could be done with the array_tpl too ...
So again a mixed response, sorry. Also, for debug node children check only, you would need to rely on the preprocessor ...
Thanks for the review. I appreciate you spending your time on it.
Do you think it is worth guarding against malformed paks? Because if not - then my changes are really not useful. I think simutrans must guard against malformed paks. pak is not a part of the program, it is runtime data, that is determined by users actions and environment (internet). Pakset downloader has closed list of source URLs hardcoded, but it is http and nobody can know for sure what will be read from unencrypted HTTP connection.
Edit: I'm asking this to know should I improve on the codestyle and performance in my patch, or is this pointless, because it solves a nonexistent problem.
refined and more focused version
I think guarding against malformed paks is not needed. It may crash simutrans
BUT- Simutrans does not run with any special privileges (not much to gain)
- Userbase is small, we have very few servers around ... (not much to gain)
- It would take a lot of effort for an exploit which will likely not work in the next version or with a slightly different compiler (i.e. the steam executable are built differently with different compilers from the normal and the flatpak releases) So any exploit would only work on a very small number.
- The target audience of Simutrans is too small. A bad actor would be noticed quickly. Since we have full control over the serverlist, sources and distribution, we could quickly exclude such sites.
The paksetdownloadeder routines download a zip file (or use an external installer on windows and shell scripts on linux/Mac), and the client game is either a bz, Z, or usually a zstd file. Targeting the bz2lib, libzstd or zlib/libz seems way more easier and likely to succeed, since hosting a fake server and sending out fake game files using a common exploit in those libaries seems the likeliest way if one ever wants to online attack simutrans.
I think your patch is fine; I would just replace auto by node_body_t (and add a _t to the class to keep with simutrans style). The type is known after all.
Furthermore, now with this in place it opens up for further optimisations. Liek allocing on the stack or using a static buffer. Bot might lead to considerably speeding up the pakset loading as the countless allocations and deletions are quite slow.
new patch has no auto. it appears in the file itself, but is removed immediately. the file contains three or so commits. should i squash them?
squashed version with node_body_t
I did a lot of tests. Using a static buffer, the routine is almost as fast as the original routine, just about 8% slower (of course in debugmode, so with release builds, it might be even faster).
Comitted the slightly modified routines using the static buffer.
There was still typo for the uint64 overread check, this is fixed in r11990.
Some performance gain by checking whole pixel block range at once.
Thanks, it gut a small (<5%) speedup.
I made all members static (since it is guarded against reentrances) and with reusing the buffer, the speedup was by 50% from 15s to about 10-11s for pak192 (when in the cache, i.e. loaded once already.) So we are now way below the baseline. Nice.
Unfortunately, the main speed limit is the file system reading the files the first time ...
Quote from: prissi on May 31, 2026, 04:49:12 AMThanks, it gut a small (<5%) speedup.
I made all members static (since it is guarded against reentrances) and with reusing the buffer, the speedup was by 50% from 15s to about 10-11s for pak192 (when in the cache, i.e. loaded once already.) So we are now way below the baseline. Nice.
Unfortunately, the main speed limit is the file system reading the files the first time ...
In pak128.german we speed up a lot, packing paks together to big paks.
for-loop with local pointer is faster than while-loop with static pointer by about 200-400ms on pak192 in my tests when compiled with nodebug and o2/o3 (compiler can use sse2 or other black magic)
not sure if worth the hassle
I think this strongly depends on the cleverness of the compiler. But you had the right idea. Since we know the endianness, and if it is not big endian, we can use memcpy, which the compiler usually inlines. That got the time down for me by another 40% to 7s for pak129.comic loading the second time (so it is in the disc cache).
Thank you for the good ideas.
r11997 introduced a read on an uninitialized pointer in obj_reader.h:
#ifndef BIG_ENDIAN
memcpy(dest, ptr, 2 * n);
ptr = cpy_end;
#else
uint8* p;
while (ptr < cpy_end) {
uint16 v = *p++;
v |= (uint16)*p++ << 8;
*dest++ = v;
}
ptr = p;
#endif
I thought that most platforms apart from PowerPC Mac use little endian nowadays. So thank you for testing this.
There still is the issue that ptr is used in the while loop, not p. Thus the loop runs infinitely and eventually segfaults.
I changed that ptr to p and tested, the program works this time.
By the way, wouldn't using ptr directly be better than creating a new pointer?
REV:12002(Linux)
Crash bevor loading pak.
QuoteProgram terminated with signal SIGSEGV, Segmentation fault.
#0 node_body_t::read_uint16_block (this=<synthetic pointer>, dest=0x3f4f2128, n=33) at src/simutrans/descriptor/reader/obj_reader.h:181
181 *dest++ = v;
#1 image_reader_t::read_node (this=<optimized out>, fp=<optimized out>, node=...) at src/simutrans/descriptor/reader/image_reader.cc:97
#2 0x0000000000479f38 in pakset_manager_t::read_nodes (fp=0x3f467270, data=@0x3fd96b00: 0x3fda7a16, node_depth=3, version=1003)
at src/simutrans/dataobj/pakset_manager.cc:353
#3 0x000000000047a0e1 in pakset_manager_t::read_nodes (fp=0x3f467270, data=@0x3fd9a190: 0x3fd9aa40, node_depth=2, version=1003)
at src/simutrans/dataobj/pakset_manager.cc:363
#4 0x000000000047a504 in pakset_manager_t::read_nodes (node_depth=1, fp=0x3f467270, data=<optimized out>, version=<optimized out>)
at src/simutrans/dataobj/pakset_manager.cc:363
#5 pakset_manager_t::read_nodes (node_depth=0, fp=0x3f467270, data=@0x7ffc1f6d5c60: 0x3fd9a140, version=<optimized out>)
at src/simutrans/dataobj/pakset_manager.cc:363
#6 pakset_manager_t::load_pak_file (filename="modern.pak") at src/simutrans/dataobj/pakset_manager.cc:207
#7 0x00000000005a8a42 in gui_theme_t::themes_init (file_name=file_name@entry=0x8ee4c6 "themes.tab", init_fonts=init_fonts@entry=true,
init_tools=init_tools@entry=false) at src/simutrans/gui/gui_theme.cc:434
#8 0x00000000007e68ec in simu_main (argc=argc@entry=2, argv=argv@entry=0x3f44d710) at src/simutrans/simmain.cc:999
#9 0x00000000007ee000 in sysmain (argc=2, argv=<optimized out>) at src/simutrans/sys/simsys.cc:1541
#10 0x00007f4c2742b33e in __libc_start_call_main () from /lib64/libc.so.6
#11 0x00007f4c2742b46b in __libc_start_main_impl () from /lib64/libc.so.6
#12 0x0000000000434ca5 in _start () at ../sysdeps/x86_64/start.S:115
Apparently some library defines BIG_ENDIAN even on little endian systems when compiling on linux, so we better use SIM_BIG_ENDIAN. (I think this was why we added this in the first place).
Okay, that's it. :o
I wouldn't have thought of that.