Undefined behavior due to player x coordinate out of range when starting a new game

[hexagonrecursion]

Describe the bug

clang UndefinedBehaviorSanitizer complains when starting a game:

src/lightmap.cpp:1120:9: runtime error: index -1 out of bounds for type 'float [132][132]'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/lightmap.cpp:1120:9 in 

Steps To Reproduce

Steps to reproduce the behavior:

1. Compile with clang -fsanitize=undefined
2. Start a new game with a "custom character"
3. See the message on the console

Expected behavior

Accessing an array out of bounds is undefined behavior. This should not happen.

Versions and configuration

• OS: Linux
  • OS Version: Fedora 30
• Game Version: 0.D-10739-gf8ba267d1c [64-bit]
• Graphics Version: Tiles
• Mods loaded: [
  Dark Days Ahead [dda],
  Disable NPC Needs [no_npc_food]
  ]

Additional context

The UB is triggered because in the following code origin.x is -1

Cataclysm-DDA/src/lightmap.cpp

Lines 1117 to 1124 in 5b3dcd7

	if( !fov_3d ) {
	std::uninitialized_fill_n(
	&seen_cache[0][0], map_dimensions, light_transparency_solid );
	seen_cache[origin.x][origin.y] = LIGHT_TRANSPARENCY_CLEAR;
	castLightAll<float, float, sight_calc, sight_check, update_light, accumulate_transparency>(
	seen_cache, transparency_cache, origin.xy(), 0 );
	} else {

build_seen_cache is called from here

Cataclysm-DDA/src/map.cpp

Lines 7702 to 7707 in 869f9b4

	// Initial value is illegal player position.
	static tripoint player_prev_pos;
	if( seen_cache_dirty || player_prev_pos != p ) {
	build_seen_cache( g->u.pos(), zlev );
	player_prev_pos = p;
	}

I think I tracked down the code responsible for setting player x position to -1

Cataclysm-DDA/src/newcharacter.cpp

Lines 746 to 755 in d136e46

	// There is no map loaded currently, so any access to the map will
	// fail (player::suffer, called from player::reset_stats), might access
	// the map:
	// There are traits that check/change the radioactivity on the map,
	// that check if in sunlight...
	// Setting the position to -1 ensures that the INBOUNDS check in
	// map.cpp is triggered. This check prevents access to invalid position
	// on the map (like -1,0) and instead returns a dummy default value.
	u.setx( -1 );
	u.reset();

The following questions are yet to be answered:

• Is the above code really the reason x is -1 or is there other code that is doing it?
• What, precisely, was the line u.reset() trying to accomplish?
• Is the line u.setx( -1 ) still necessary now (years later)?
• Is the line u.setx( -1 ) still sufficient?
• Has anyone added code that depends on the bogus value of player x coordinate or can simply adding u.setx( 0 ) below u.reset() fix everything without breaking something else and starting the vicious cycle all over again?

[BevapDin]

Setting x to 0 after the call to reset should be fine. The setting is almost certainly still needed.

---

But: why is the lightmap generated before the player is placed on the map (hat step give the player a valid location)? Is the lightmap created during character creation?

---

Ideally, we would have a separate data structure that stores the settings done during character creation (traits, profession, ...). This would be stored as template. And it would calculate the stats (to display and to use when the character instance is created) based on that.

[hexagonrecursion]

Is the lightmap created during character creation?

No. u.setx( -1 ) is called from set_stats, which is invoked during character creation when you tab to the "stats" tab of the character creation UI. It is not triggered if you select "play now (fixed scenario)" or "play now". It is not triggered if you select "random character" unless you tab back to the "stats" tab. Here is the stack trace:

(lldb) bt
* thread #1, name = 'cataclysm-tiles', stop reason = breakpoint 1.1
  * frame #0: 0x0000000004d2c64f cataclysm-tiles`set_stats(w=<unavailable>, u=<unavailable>, points=<unavailable>) at newcharacter.cpp:734
    frame #1: 0x0000000004d12711 cataclysm-tiles`avatar::create(this=0x000062b00001c200, type=PLTYPE_CUSTOM, tempname=0x0000000000000000) at newcharacter.cpp:431:26
    frame #2: 0x000000000418e9fe cataclysm-tiles`main_menu::new_character_tab(this=<unavailable>) at main_menu.cpp:792:31
    frame #3: 0x000000000418a3cf cataclysm-tiles`main_menu::opening_screen(this=<unavailable>) at main_menu.cpp:539:37
    frame #4: 0x000000000416e57a cataclysm-tiles`main(argc=<unavailable>, argv=0x00007fffffffe0c8) at main.cpp:683:23
    frame #5: 0x00007ffff78d5f43 libc.so.6`__libc_start_main(main=(cataclysm-tiles`main at main.cpp:135), argc=1, argv=0x00007fffffffe0c8, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffe0b8) at libc-start.c:308:16
    frame #6: 0x000000000204c1ce cataclysm-tiles`_start + 46

map::build_seen_cache is invoked later (after you confirm character creation) while a message "please wait as we build your world" is displayed. Here is the stack trace:

(lldb) bt
* thread #1, name = 'cataclysm-tiles', stop reason = breakpoint 2.1
  * frame #0: 0x0000000003f31b45 cataclysm-tiles`map::build_seen_cache(this=0x0000617000003f80, origin=0x000062b00001edf8, target_z=0) at lightmap.cpp:1106
    frame #1: 0x00000000042525f6 cataclysm-tiles`map::build_map_cache(this=0x0000617000003f80, zlev=0, skip_lightmap=<unavailable>) at map.cpp:7705:9
    frame #2: 0x00000000033786c3 cataclysm-tiles`game::start_game(this=0x00006190000c1780) at game.cpp:728:7
    frame #3: 0x000000000418ea8a cataclysm-tiles`main_menu::new_character_tab(this=<unavailable>) at main_menu.cpp:805:29
    frame #4: 0x000000000418a3cf cataclysm-tiles`main_menu::opening_screen(this=<unavailable>) at main_menu.cpp:539:37
    frame #5: 0x000000000416e57a cataclysm-tiles`main(argc=<unavailable>, argv=0x00007fffffffe0c8) at main.cpp:683:23
    frame #6: 0x00007ffff78d5f43 libc.so.6`__libc_start_main(main=(cataclysm-tiles`main at main.cpp:135), argc=1, argv=0x00007fffffffe0c8, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffe0b8) at libc-start.c:308:16
    frame #7: 0x000000000204c1ce cataclysm-tiles`_start + 46