New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCI: implement strncpy with memmove #1030

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@moralrecordings

moralrecordings commented Sep 24, 2017

The C spec says that for strcpy and strncpy, the source and destination cannot have overlapping buffers. If memory serves, sometime back in 2011 glibc was changed on x86 to copy memory backwards, because of some alleged infinitessimal speed boost. Since then, any code that assumed a strcpy with overlapping buffers would work because the source was before the destination will very subtly break.

There's at least one instance of a SCI game expecting strcpy to work with overlapping buffers, so this patch replaces the underlying call with a strlen() + memmove(), which is guaranteed to work.

Fixes the flamingo puzzle in Island of Dr. Brain v1.1 on Linux.

@moralrecordings

This comment has been minimized.

Show comment
Hide comment
@moralrecordings

moralrecordings Sep 24, 2017

For kicks I wrote up the troubleshooting process here - https://moral.net.au/writing/2017/09/23/sierra_bug/

moralrecordings commented Sep 24, 2017

For kicks I wrote up the troubleshooting process here - https://moral.net.au/writing/2017/09/23/sierra_bug/

@wjp

This comment has been minimized.

Show comment
Hide comment
@wjp

wjp Sep 24, 2017

Member

Great catch, thanks! Very interesting write-up too.

By the way, our data file hashes are only of the first 5000 bytes, which may be why it didn't appear to match for you.

Could you add a reference to the SCI script method that depends on this behaviour (flamingo::init in Island of Dr Brain, it seems?) in the commit message? That'll make it easier to test in the future if we ever want to change these functions.

This might also affect the memcpy function in a similar way.

A complicating matter is that memmove might not always do what the original Sierra interpreters did for overlapping memory areas. From this flamingos case, it seems to do the right thing when copying overlapping strings forward, but we need to test (or reverse) if this approach matches SSCI when copying strings backward too. (From a quick glance, KQ5 copies bytes in a forward loop.)

Member

wjp commented Sep 24, 2017

Great catch, thanks! Very interesting write-up too.

By the way, our data file hashes are only of the first 5000 bytes, which may be why it didn't appear to match for you.

Could you add a reference to the SCI script method that depends on this behaviour (flamingo::init in Island of Dr Brain, it seems?) in the commit message? That'll make it easier to test in the future if we ever want to change these functions.

This might also affect the memcpy function in a similar way.

A complicating matter is that memmove might not always do what the original Sierra interpreters did for overlapping memory areas. From this flamingos case, it seems to do the right thing when copying overlapping strings forward, but we need to test (or reverse) if this approach matches SSCI when copying strings backward too. (From a quick glance, KQ5 copies bytes in a forward loop.)

@moralrecordings

This comment has been minimized.

Show comment
Hide comment
@moralrecordings

moralrecordings Sep 24, 2017

Can do. I was actually about to panic and drop the request, as after rebasing restoring save games made the SCI interpreter crash, but that appears to have been caused by an unrelated commit chain from yesterday?

I thought about memcpy, but was hesitant to go ahead and change something there wasn't a concrete test case for. Easy enough to do, just replace ::memcpy with ::memmove. As for the reverse case... offhand I can't think of a scenario where you'd want the weird inconsistent strncpy output. I think someone with a better understanding of SCI internals would have to weigh in on how the real engine implements it.

moralrecordings commented Sep 24, 2017

Can do. I was actually about to panic and drop the request, as after rebasing restoring save games made the SCI interpreter crash, but that appears to have been caused by an unrelated commit chain from yesterday?

I thought about memcpy, but was hesitant to go ahead and change something there wasn't a concrete test case for. Easy enough to do, just replace ::memcpy with ::memmove. As for the reverse case... offhand I can't think of a scenario where you'd want the weird inconsistent strncpy output. I think someone with a better understanding of SCI internals would have to weigh in on how the real engine implements it.

@csnover

This comment has been minimized.

Show comment
Hide comment
@csnover

csnover Sep 24, 2017

Member

Can do. I was actually about to panic and drop the request, as after rebasing restoring save games made the SCI interpreter crash, but that appears to have been caused by an unrelated commit chain from yesterday?

I’m not able to reproduce a crash when restoring Island of Dr Brain save games when building from master. (I’m also not sure how this changeset could cause such a problem either.) Could you provide more detail on what you are experiencing? Disregard this, I am able to reproduce this now.

Member

csnover commented Sep 24, 2017

Can do. I was actually about to panic and drop the request, as after rebasing restoring save games made the SCI interpreter crash, but that appears to have been caused by an unrelated commit chain from yesterday?

I’m not able to reproduce a crash when restoring Island of Dr Brain save games when building from master. (I’m also not sure how this changeset could cause such a problem either.) Could you provide more detail on what you are experiencing? Disregard this, I am able to reproduce this now.

@csnover

This comment has been minimized.

Show comment
Hide comment
@csnover

csnover Sep 24, 2017

Member

Sorry about the save game problem, the serialiser was losing its version information. It should be OK again, though you will need to regenerate any save game made in the last half-day or so. :\

Member

csnover commented Sep 24, 2017

Sorry about the save game problem, the serialiser was losing its version information. It should be OK again, though you will need to regenerate any save game made in the last half-day or so. :\

@csnover csnover closed this Sep 27, 2017

@csnover

This comment has been minimized.

Show comment
Hide comment
@csnover

csnover Sep 27, 2017

Member

Oops. I was trying to update the PR with an alternate implementation using an explicit forward copy, and GitHub accepted most of the push but then decided the replacement commit wasn’t valid and closed the PR. I can’t reopen this now, so I guess I will just open a new one with the different patch. Thanks, @moralrecordings, for your great write-up, work on isolating the problem, and the initial patch!

Member

csnover commented Sep 27, 2017

Oops. I was trying to update the PR with an alternate implementation using an explicit forward copy, and GitHub accepted most of the push but then decided the replacement commit wasn’t valid and closed the PR. I can’t reopen this now, so I guess I will just open a new one with the different patch. Thanks, @moralrecordings, for your great write-up, work on isolating the problem, and the initial patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment