Show Your Working: Patching a bug in a 25 year old Sierra game
23 September 2017
Show Your Working is a brand new segment where I write up things I have attempted to fix in open source software. Sometimes it's interesting to debug a problem yourself from first principles, even if the codebase is huge and you don't know anything going in. I will try and explain my thought process as I venture out into the weeds armed only with a butter knife.
The software
ScummVM is a heroic effort to make a cross-platform engine for hundreds of classic adventure games, making them natively playable on modern systems. Since 2010, ScummVM contains an interpreter originally from the FreeSCI project, and by now does a pretty good job playing games based on the Sierra Creative Interpreter. If you haven't heard of ScummVM, I can't recommend it enough.
It's gotten to the point where ScummVM provides a much better experience than emulating the games in DOS/Windows. Still, given that the engine is reverse engineered without the original code, there's all manner of edge cases and quirks that have to be emulated in order for the games to work correctly, as we'll find out!
The problem
The old Sierra mantra was "save early, save often"; and not just because a game could be rendered unwinnable by a moon-logic puzzle requiring a 1px * 1px item from the first screen you didn't pick up. Or a bad arcade sequence tied to the CPU clock, playing at insane speeds on anything faster than a 386. Or the frequent risk of clicking the wrong spot and watching your character have all their skin eaten away. No, there are bugs and race conditions in the scripting code that makes up these games! Quite a lot of these bugs are overridden/ignored by the build of SCI the game was shipped with, but other times you aren't that lucky and the whole game tanks, wiping out whatever progress you've made.
In case you're not familiar, the original Sierra crash screen looked like this:
Oh man do not get me started on this message. Protip for designers: if your software crashes, don't be condescending ("of course doing that thing would break the game, what are you taking stupid pills???") and don't lie to save face (plenty of crashes can be triggered by doing the correct thing and you damn well know it Sierra). 7 year old me felt worse than he should have because of that.
Okay, some specifics. I felt compelled to replay Island of Dr. Brain, an edutainment game for ages 12 and up, to rack up a large score. (Don't judge.) I got as far as that accursed puzzle with the colour changing flamingos, when ScummVM threw open the debug console.
The preparation
The only software we need installed to debug this is the Git VCS and the standard Linux command line tools, plus whatever build dependencies ScummVM has. Start by grabbing the latest ScummVM source code.
$ git clone https://github.com/scummvm/scummvm
$ cd scummvm
The diagnosis
The ScummVM source tree is very modular, with each engine compartmentalised from the rest of the cross-platform code. All the stuff we're going to want is in the SCI engine folder /engines/sci.
Let's start with the autodetection. ScummVM does some very basic game auto-detection by MD5ing the first 5000 bytes of files in the game directory. The detection tables are in /engines/sci/detection_tables.h
// The Island of Dr. Brain - English DOS (from Quietust)
// Executable scanning reports "1.001.053", VERSION file reports "1.1 2.3.93"
{"islandbrain", "", {
{"resource.map", 0, "3c07da06bdd1689f9d07af78fb94d0ec", 3101},
{"resource.000", 0, "ecc686e0034fb4d41de077ac7167b3cf", 1947866},
AD_LISTEND},
Common::EN_ANY, Common::kPlatformDOS, 0, GUIO4(GUIO_NOSPEECH, GAMEOPTION_PREFER_DIGITAL_SFX, GAMEOPTION_ORIGINAL_SAVELOAD, GAMEOPTION_FB01_MIDI) },
This is the entry that matches my copy. The 5000 byte hashes for resource.map and resource.000 are correct, and date in the version file is correct. Now, let's see if there's any specific modifications made for the game:
[email protected]:~/Development/scummvm/engines/sci$ grep -rni islandbrain
engine/workarounds.cpp:305: { GID_ISLANDBRAIN, 100, 937, 0, "IconBar", "dispatchEvent", NULL, 58, { WORKAROUND_FAKE, 0 } }, // when using ENTER at the startup menu - bug #5241
engine/workarounds.cpp:306: { GID_ISLANDBRAIN, 140, 140, 0, "piece", "init", NULL, 3, { WORKAROUND_FAKE, 1 } }, // first puzzle right at the start, some initialization variable. bnt is done on it, and it should be non-0
engine/workarounds.cpp:307: { GID_ISLANDBRAIN, 200, 268, 0, "anElement", "select", NULL, 0, { WORKAROUND_FAKE, 0 } }, // elements puzzle, gets used before super TextIcon
engine/workarounds.cpp:550: { GID_ISLANDBRAIN, 300, 300, 0, "geneDude", "show", NULL, 0, { WORKAROUND_IGNORE, 0 } }, // when looking at the gene explanation chart - a parameter is an object
engine/workarounds.cpp:648: { GID_ISLANDBRAIN, 300, 300, 0, "dudeViewer", "show", NULL, 0, { WORKAROUND_STILLCALL, 0 } }, // when looking at the gene explanation chart, gets called with 1 extra parameter
engine/workarounds.cpp:670: { GID_ISLANDBRAIN, 290, 291, 0, "upElevator", "changeState", sig_kGraphSaveBox_ibrain_1, 0, { WORKAROUND_STILLCALL, 0 } }, // when testing in the elevator puzzle, gets called with 1 argument less - 15 is on stack - bug #4943
engine/workarounds.cpp:671: { GID_ISLANDBRAIN, 290, 291, 0, "downElevator", "changeState", sig_kGraphSaveBox_ibrain_1, 0, { WORKAROUND_STILLCALL, 0 } }, // see above
engine/workarounds.cpp:672: { GID_ISLANDBRAIN, 290, 291, 0, "correctElevator", "changeState", sig_kGraphSaveBox_ibrain_1, 0, { WORKAROUND_STILLCALL, 0 } }, // see above (when testing the correct solution)
engine/workarounds.cpp:741: { GID_ISLANDBRAIN, -1, 999, 0, "List", "eachElementDo", NULL, 0, { WORKAROUND_FAKE, 0 } }, // when going to the game options, choosing "Info" and selecting anything from the list, gets called with an invalid parameter (type "error") - bug #4989
engine/workarounds.cpp:869: { GID_ISLANDBRAIN, 300, 310, 0, "childBreed", "changeState", sig_kStrAt_ibrain_1, 0, { WORKAROUND_FAKE, 0 } }, // when clicking Breed to get the second-generation cyborg hybrid (Standard difficulty), the two parameters are swapped - bug #5088
...
Yup. There's a few workarounds on file for this. Here's the C struct format for each workaround, taken from /sci/engine/workarounds.h:
enum SciWorkaroundType {
WORKAROUND_NONE, // only used by terminator or when no workaround was found
WORKAROUND_IGNORE, // ignore kernel call
WORKAROUND_STILLCALL, // still do kernel call
WORKAROUND_FAKE // fake kernel call / replace temp value / fake opcode
};
struct SciWorkaroundSolution {
SciWorkaroundType type;
uint16 value;
};
/**
* A structure describing a 'workaround' for a SCI script bug.
*
* Arrays of SciWorkaroundEntry instances are terminated by
* a fake entry in which "objectName" is NULL.
*/
struct SciWorkaroundEntry {
SciGameId gameId;
int roomNr;
int scriptNr;
int16 inheritanceLevel;
const char *objectName;
const char *methodName;
const uint16 *localCallSignature;
int index;
SciWorkaroundSolution newValue;
};
Seems pretty straightforward. Our game is GID_ISLANDBRAIN, and from that stack trace we know the room number is 180 and the script number is 185. Inheritance level I assume is 0, and again from the stack trace objectName is "flamingo" and methodName is "showHelp". index I assume for now is 0. localCallSignature is more interesting... in SCI code there are subroutines that can get called without names, and we need a way of identifying them without just saying "the code starts from this offset", as that isn't robust for multiple versions of the same game. Our stack trace was blaming localCall 5d6 for doing an invalid command, so we'll possibly need to identify it. newValue will be the workaround action we'll ask ScummVM to shim in, which we'll find out once we troubleshoot.
As this is an emulator, we are going to be using the internal debugger a lot. Internal debuggers are pretty daunting, as tradition dictates they have barely any documentation, plus you're grappling with some weirdo niche instruction set maybe 10 people know properly. That's ok! Most internal debuggers list all the allowed commands with "help", and as long as you have a very basic understanding of how assembled programs work (i.e. registers, pushing and popping things on the stack, jumps, conditionals), you can do pretty much anything you want in a debugger as soon as you find out how to do 3 things:
- Look at the current state. For ScummVM's SCI engine, this is "registers" to see all the registers, "stack 10" to see the first 10 things pushed on the stack, "vm_vars [local|global|temp|param] id" to view a variable.
- Run/step in/step over code. For ScummVM's SCI engine, this is "go" to continue execution, "s" to step in and "p" to step over.
- Set code breakpoints. For ScummVM's SCI engine, the two I used were "bp_method object::methodName" to break at the start of a named method, and "bp_address [segment]:[offset]" to stop at a section of code.
# in ScummVM debug console
> disasm flamingos showHelp bc
...
0036:01b5: b5 01 sati 01
0036:01b7: 76 push0
0036:01b8: 85 00 lat 00
0036:01ba: b3 1c sali 1c ; 28
0036:01bc: c5 00 +at 00
0036:01be: 33 ea jmp ea [01aa]
0036:01c0: 76 push0
0036:01c1: 40 11 04 00 call 0411 [05d5] 00
0036:01c5: 78 push1
0036:01c6: 39 14 pushi 14 ; 20, name
0036:01c8: 40 48 03 02 call 0348 [0513] 02
0036:01cc: 35 00 ldi 00
0036:01ce: a5 00 sat 00
0036:01d0: 8d 00 lst 00
0036:01d2: 39 04 pushi 04 ; cel
0036:01d4: 81 72 lag 72 ; 114, 'r'
0036:01d6: 02 add
0036:01d7: 22 lt?
0036:01d8: 31 1c bnt 1c [01f6]
...
In the stack trace, the second last line said the program counter for the bad call was 0036:01c5. To make sense of this, I recommend having a look in /engines/sci/engine/vm.cpp, which contains the opcode list.
Annoyingly I couldn't figure out how to make ScummVM disassemble the local method, so I had to rely on a third-party tool SCICompanion to do the dirty work and disassemble all of script 185.
05d6:3f 02 link 2 // (var $2)
05d8:35 00 ldi 0
05da:a5 00 sat temp0
code_05dc
05dc:8d 00 lst temp0
05de:35 09 ldi 9
05e0:22 lt?
05e1:31 22 bnt code_0605
05e3:39 03 pushi 3 // $3 loop
05e5:78 push1
05e6:85 00 lat temp0
05e8:9b 1c lsli local28
05ea:39 03 pushi 3 // $3 loop
05ec:76 push0
05ed:93 00 lali local0
05ef:4a 04 send 4
05f1:36 push
05f2:35 01 ldi 1
05f4:12 and
05f5:02 add
05f6:36 push
05f7:38 00d9 pushi d9 // $d9 show
05fa:76 push0
05fb:85 00 lat temp0
05fd:93 00 lali local0
05ff:4a 0a send a
0601:c5 00 +at temp0
0603:33 d7 jmp code_05dc
code_0605
0605:48 ret
05ef is where the last line of the stack trace claims the code stops executing. From my very tenuous understanding of the error, send is freaking out because the argument passed in through the accumulator register (0000:0000) isn't an object. And the lali instruction sets the accumulator to the value of local variable local0.
# in ScummVM console
> acc_object
Information on the currently active object or class at the address indexed by the accumulator:
[0000:0000]: Not an object.
> vm_vars local 0
local var 0 = 0027:0eb9 (object 'bird')
Uhhhhh. What. If local0 is not a null, why did that break?!
Okay. Reloading the game up to before the crash, I opened the console with CTRL + LSHIFT + D, and added a breakpoint in the broken method. I then triggered the puzzle and set a breakpoint for the code just before the crash, using the segment from the program counter.
# in ScummVM console
> bp_method flamingos::showHelp
#0: Execute flamingos::showHelp
> exit
# open the puzzle
Break on flamingos::showHelp (in [0036:075c])
flamingos::showHelp(0000:0000) at 0036:019b
> registers
Current register values:
acc=0000:0001 prev=0000:0001 &rest=0
pc=0036:019b obj=0036:075c fp=ST:0036 sp=ST:0036
> bp_address 0036:05ec
> go
# click through
> acc_object
Information on the currently active object or class at the address indexed by the accumulator:
[0000:0000]: Not an object.
> vm_vars local 0
local var 0 == 0027:0eb8 (object 'bird')
# skip to 05ef
> p
> p
> acc_object
Information on the currently active object or class at the address indexed by the accumulator:
[0027:0eb8] bird : 34 vars, 2 methods
...
So the first time around we're entering the send instruction exactly as we should, with the bird object in the accumulator. This subroutine loops a few times, and every test run it crashed at a different iteration of the loop. Which I 100% do not get, as there is nothing in the subroutine that would set acc to the wrong thing! Could... could it be that the bug was in ScummVM all along?
Looks like we have to go a level deeper. If we run ScummVM inside gdb and stop it just before we execute the lali instruction, we can add a breakpoint to the code responsible for lali.
(gdb) b engines/sci/engine/vm.cpp:1284
Breakpoint 1 at 0x5555567b9750: file engines/sci/engine/vm.cpp, line 1284.
case op_lag: // 0x40 (64)
case op_lal: // 0x41 (65)
case op_lat: // 0x42 (66)
case op_lap: // 0x43 (67)
// Load global, local, temp or param variable into the accumulator
case op_lagi: // 0x48 (72)
case op_lali: // 0x49 (73)
case op_lati: // 0x4a (74)
case op_lapi: // 0x4b (75)
// Same as the 4 ones above, except that the accumulator is used as
// an additional index
var_type = opcode & 0x3; // Gets the variable type: g, l, t or p
var_number = opparams[0] + (opcode >= op_lagi ? s->r_acc.requireSint16() : 0);
s->r_acc = read_var(s, var_type, var_number);
break;
Probing the variables with gdb, I realised I was mistaken. The "lali [arg]" instruction doesn't load local variable [arg] into acc, it loads local variable [arg+acc] into acc. And at that part of the code acc (stored on the stack) increases by 1 on each loop. Like an idiot I forgot the puzzle has 9 birds in it.
# in ScummVM console
> vm_vars local 0
local var 0 = 0027:0e09 (object 'bird')
> vm_vars local 1
local var 1 = 0027:0e07 (object 'bird')
> vm_vars local 2
local var 2 = 0027:0e04 (object 'bird')
> vm_vars local 3
local var 3 = 0027:0e03 (object 'bird')
> vm_vars local 4
local var 4 = 0027:0e0a (object 'bird')
> vm_vars local 5
local var 5 = 0027:0e09 (object 'bird')
> vm_vars local 6
local var 6 = 0000:0000
> vm_vars local 7
local var 7 = 0027:0e06 (object 'bird')
> vm_vars local 8
local var 8 = 0027:0e0b (object 'bird')
Well there's your problem. But where did that 7th bird go? Better yet, on other runs there are multiple birds missing from the list at very different spots. In a twist the showHelp command seems to be innocent, and it's really a setup process somewhere else that's botching up! Thankfully we only have one short disassembly to look through for the culprit (script 185), and from vm.cpp there's only a few opcodes which can set local variables.
Sure enough, in the flamingos::init method, there's an instruction "sali local0". I throw a breakpoint on it, then quizzed the VM for information before and after.
# in ScummVM console
> bp_method flamingos::init
...
> bp_address 0036:00ac
> go
...
> registers
Current register values:
acc=0000:0006 prev=0000:0009 &rest=0
pc=0036:00ac obj=0036:075c fp=ST:002b sp=ST:0042
> stack 5
ST:003d = 0000:0001
ST:003e = 0000:0006
ST:003f = 0000:0075
ST:0040 = 0000:0000
ST:0041 = 0027:0ec0
> p
> registers
Current register values:
acc=0027:0ec0 prev=0000:0009 &rest=0
pc=0036:00ae obj=0036:075c fp=ST:002b sp=ST:0041
> stack 5
ST:003c = 0000:003d
ST:003d = 0000:0001
ST:003e = 0000:0006
ST:003f = 0000:0075
ST:0040 = 0000:0000
As expected (after reading vm.cpp), "sali local0" pops the bird object from the top of the stack, and stores it at variable local[0+acc]. I did this for each iteration of the loop, and found something strange. I was expecting acc to randomly cycle through the numbers from 0 to 8 with each bird getting a slot, but sometimes the same number was coming up multiple times, which would overwrite a variable and leave a gap. Which leads to my next question; how is acc chosen?
00a1:8b 1b lsl local27
00a3:8d 00 lst temp0
00a5:43 62 04 callk StrAt 4 // acc = StrAt(local[27], temp[0])
00a8:36 push //
00a9:35 61 ldi 61 //
00ab:04 sub // acc -= 0x61
00ac:b3 00 sali local0 // index = 0+acc; acc = pop(); local[index] = acc
I really didn't get this at first. What the hell is StrAt? Why the arbitrary 0x61 offset?
It's not that bad. "callk" is the opcode for a so-called kernel call, which is basically predefined C code in the SCI VM. It took me way too long to realise, but acc = StrAt(string, index) is the equivalent of acc = string[index], which seems ridiculous but okay. And what was the string, I hear you ask?
Thread 1 "scummvm" hit Breakpoint 1, Sci::kStrAt (s=0x555558dff180, argc=2, argv=0x555558e7292c) at engines/sci/engine/kstring.cpp:88
88 if (argv[0] == SIGNAL_REG) {
(gdb) l
83 return argv[0];
84 }
85
86
87 reg_t kStrAt(EngineState *s, int argc, reg_t *argv) {
88 if (argv[0] == SIGNAL_REG) {
89 warning("Attempt to perform kStrAt() on a signal reg");
90 return NULL_REG;
91 }
92
(gdb) p argv
$1 = (Sci::reg_t *) 0x555558e7292c
(gdb) p argv[0]
$2 = {
_segment = 54,
_offset = 2100
}
(gdb) p argv[1]
$3 = {
_segment = 0,
_offset = 0
}
(gdb) p argc
$4 = 2
(gdb) n
93 SegmentRef dest_r = s->_segMan->dereference(argv[0]);
(gdb) n
94 if (!dest_r.isValid()) {
(gdb) p dest_r
$5 = {
isRaw = true,
{
raw = 0x555558fa70a4 "cdhfagbbi",
reg = 0x555558fa70a4
},
maxSize = 54,
skipByte = false
}
Oh. Ohhhhhh. So there's ANOTHER method that's generating a 9 character string with the letters from a to i to indicate the bird position in the variables list, and storing the pointer to it in local[27]. 0x61 is the ASCII character 'a', so subtracting that from each letter gives an index from 0 to 8. Does that seriously mean the SCI VM doesn't have any array types other than byte strings? If so it's pretty wild they made all these math based puzzles.
# in ScummVM console
> bp_method flamingos::init
#0: Execute flamingos::init
> exit
# open the puzzle
Break on flamingos::init (in [0036:075c])
flamingos::init() at 0036:002c
> vm_vars local 27
local var 27 == 0036:0834 (reference)
> view_reference 0036:0834
0036:0834 is of type 0x10: raw data
000000: 61 62 63 64 65 66 67 68 69 00 52 65 73 65 74 74 |abcdefghi.Resett|
000010: 69 6e 67 00 66 6c 61 6d 69 6e 67 6f 73 00 66 6c |ing.flamingos.fl|
000020: 61 6d 57 69 6e 00 62 69 72 64 00 00 04 00 3a 00 |amWin.bird....:.|
000030: 18 01 72 01 aa 01 |..r... |
flamingos::init() starts with local[27] pointing to the pre-baked string 'abcdefghi', then does some operations to shuffle the contents around. This process fails and overwrites parts of the string 10-20% of the time. In fact, the 6th instruction in flamingos::init() throws to a subroutine, and checking before/after we know that subroutine is responsible for doing the shuffle.
Because I don't know the instruction set that well, usually I go through and translate each instruction to the C/Python equivalent, in this case using the interpreter in vm.cpp as a guide, consolidating lines as I go. Here's the copy with my annotations:
(procedure proc_04c3 // param[1] = local[27]
04c3:3f 03 link 3 // link(3)
04c5:78 push1 // # 1 arg
04c6:8f 01 lsp param1 //
04c8:43 46 02 callk StrLen 2 // acc = StrLen(param[1])
04cb:a5 00 sat temp0 // temp[0] = acc
code_04cd
04cd:85 00 lat temp0 // acc = temp[0]
04cf:31 42 bnt code_0513 // if (acc == 0) goto code_0513
04d1:7a push2 // # 2 args
04d2:78 push1
04d3:36 push
04d4:43 3c 04 callk Random 4 // acc = Random(1, acc)
04d7:36 push //
04d8:35 01 ldi 1 //
04da:04 sub // acc -= 1
04db:a5 02 sat temp2 // temp[2] = acc
04dd:7a push2 // # 2 args
04de:8f 01 lsp param1 //
04e0:36 push //
04e1:43 62 04 callk StrAt 4 // acc = StrAt(param[1], acc)
04e4:a5 01 sat temp1 // temp[1] = acc
04e6:7a push2 // # 2 args
04e7:8f 01 lsp param1 //
04e9:85 02 lat temp2 //
04eb:02 add // acc = param[1] + temp[2]
04ec:36 push // dest = acc
04ed:8f 01 lsp param1 //
04ef:85 02 lat temp2 //
04f1:02 add // acc = param[1] + temp[2]
04f2:36 push //
04f3:35 01 ldi 1 //
04f5:02 add // acc += 1
04f6:36 push // src = acc
04f7:43 47 04 callk StrCpy 4 // strcpy(dest, src); acc = src
04fa:39 03 pushi 3 // # 3 args
04fc:8f 01 lsp param1 //
04fe:39 08 pushi 8 //
0500:8d 01 lst temp1 //
0502:43 62 06 callk StrAt 6 // acc = StrAt(param[1], 8, temp[1])
0505:39 03 pushi 3 // # 3 args
0507:8f 01 lsp param1 //
0509:39 09 pushi 9 //
050b:76 push0 //
050c:43 62 06 callk StrAt 6 // acc = StrAt(param[1], 9, 0)
050f:e5 00 -at temp0 // temp0 -= 1
0511:33 ba jmp code_04cd // if (temp0 != 0) goto code_04cd
code_0513
0513:48 ret // return
)
Easy. For 9 iterations, the shuffler picks a random point in the string, copies all the characters to the right of it one space left, then re-adds the missing character and a null to the end.
Right I think I can guess the twist, it's going to be the Random function doing something stupid. If I was a gambling man, probably Random(1, 1) will do something idiotic like return 0 and balls up the shuffling math.
# in ScummVM console
> bp_address 0036:04d4
> bp_address 0036:04f7
...
Break at 0036:4d4
> p
> registers
Current register values:
acc=0000:0001 prev=0000:0001 &rest=0
pc=0036:04d7 obj=0036:075c fp=ST:0030 sp=ST:0033
...
0036:04f7: 43 47 04 callk StrCpy[47], 04
Kernel params: (0036:0834, 0036:0835)
000000: 61 62 64 65 66 67 68 69 63 00 52 65 73 65 74 74 |abdefghic.Resett|
000010: 69 6e 67 00 66 6c 61 6d 69 6e 67 6f 73 00 66 6c |ing.flamingos.fl|
000020: 61 6d 57 69 6e 00 62 69 72 64 00 00 04 00 3a 00 |amWin.bird....:.|
000030: 18 01 72 01 aa 01 |..r... |
Step #144526
0036:04fa: 39 03 pushi 03 ; loop
000000: 62 64 65 66 67 69 63 63 00 00 52 65 73 65 74 74 |bdefgicc..Resett|
000010: 69 6e 67 00 66 6c 61 6d 69 6e 67 6f 73 00 66 6c |ing.flamingos.fl|
000020: 61 6d 57 69 6e 00 62 69 72 64 00 00 04 00 3a 00 |amWin.bird....:.|
000030: 18 01 72 01 aa 01 |..r... |
I lose. Random is working fine. It was StrCpy; the one in ScummVM's SCI engine doesn't react well when the source and destination strings overlap!
And by "the one in ScummVM's SCI engine", I mean glibc. The C spec says the behaviour in the case of overlapping is undefined, and some individuals have taken that to mean that it's 100% A-OK to change that behaviour and (why not) break every closed-source program that relies on it. In fact, this legendary bug report has those two titans of community outreach and civil discourse, Linus Torvalds and Ulrich Drepper, arguing whether or not it was fine to break Adobe Flash for an alleged infintessimal speed boost you got on x86 by copying things in reverse.
The solution
As the bug was in one of the SCI kernel functions, we didn't need to add anything to the workaround table after all. The fix was to replace the SCI kernel's underlying calls to strcpy/strncpy with strlen + memmove, which allow an overlapping source and destination buffer. Voila, awful flamingo puzzle works.
diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp
index aa70d5f838..3b65e78d52 100644
--- a/engines/sci/engine/seg_manager.cpp
+++ b/engines/sci/engine/seg_manager.cpp
@@ -624,11 +624,18 @@ void SegManager::strncpy(reg_t dest, const char* src, size_t n) {
if (dest_r.isRaw) {
+ size_t len = ::strlen(src);
// raw -> raw
- if (n == 0xFFFFFFFFU)
- ::strcpy((char *)dest_r.raw, src);
- else
- ::strncpy((char *)dest_r.raw, src, n);
+ if (n == 0xFFFFFFFFU) {
+ ::memmove((char *)dest_r.raw, src, len+1);
+ } else {
+ if (n < len) {
+ ::memmove((char *)dest_r.raw, src, n);
+ } else {
+ ::memmove((char *)dest_r.raw, src, len);
+ ::memset((char *)dest_r.raw+len, 0, n-len);
+ }
+ }
} else {
// raw -> non-raw
for (uint i = 0; i < n; i++) {