Skip to content
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

kernel: fix string copying logic #3071

Merged
merged 1 commit into from
Nov 30, 2018

Conversation

rbehrends
Copy link
Contributor

This pull request fixes two bugs in src/stringobj.c.

  1. Calling CopyToStringRep() with a string whose length had been shortened by SET_STR_LEN() or which for some other reason leaves one or more words at the end unused could overwrite random memory, as memcpy() used SIZE_OBJ() for its length.
  2. Part of the logic in ImmutableString() was inverted, copying the string if it was immutable rather than if it was mutable.

@rbehrends rbehrends added kind: bug Issues describing general bugs, and PRs fixing them topic: kernel labels Nov 29, 2018
@@ -1243,7 +1243,8 @@ Obj CopyToStringRep(
copy = NEW_STRING(lenString);

if ( IS_STRING_REP(string) ) {
memcpy(ADDR_OBJ(copy), CONST_ADDR_OBJ(string), SIZE_OBJ(string));
memcpy(CHARS_STRING(copy), CONST_CHARS_STRING(string),
GET_LEN_STRING(string));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This goes back to a255816 from Nov 2014, so it affects 4.10. On the up side, I imagine this problem cannot cause issues with GASMAN, only with Julia GC or Boehm, right? So it's not so bad (and less surprising that we never noticed).
Still, perhaps it should be backported to 4.10.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With GASMAN, the overflow should indeed generally only hit unused memory, except possibly in the case of 32-bit systems, where it can possibly overflow the reserved area.

I did track it down on HPCGAP with the Boehm GC, where the change to ImmutableString() triggered it reliably during testing.

This may also be one candidate for the spurious crashes that we have seen with the Julia GC.

@@ -1267,7 +1268,7 @@ Obj CopyToStringRep(
*/
Obj ImmutableString(Obj string)
{
if (!IS_STRING_REP(string) || !IS_MUTABLE_OBJ(string)) {
if (!IS_STRING_REP(string) || IS_MUTABLE_OBJ(string)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is much more recently, from 742f1a4 this September (and also my fault... :-/). On the upside, it is not in GAP 4.10, so not much harm done :-).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also not a critical issue. ImmutableString() is only used for the name field of functions and methods, meaning that in the worst case, that name was still mutable. Again, I ran into it during testing HPCGAP, where it led to names being in a thread-local region and thus inaccessible from other threads.

@rbehrends rbehrends merged commit df40f9f into gap-system:master Nov 30, 2018
@fingolfin
Copy link
Member

Backported the relevant bits to stable-4.10 via 872d493

@fingolfin fingolfin deleted the fix-stringobj-copy branch January 8, 2019 10:22
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.1 milestone Jan 26, 2019
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.10-DONE kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants