Skip to content

kernel: fix string copying logic #3071

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.

Sign up for GitHub

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

Merged
rbehrends merged 1 commit into from
Nov 30, 2018
Merged

kernel: fix string copying logic #3071

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/stringobj.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@fingolfin fingolfin Nov 29, 2018

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

@rbehrends rbehrends Nov 30, 2018

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.

/* XXX no error checks? */
} else {
/* copy the string to the string representation */
Expand All @@ -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

@fingolfin fingolfin Nov 29, 2018

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

@rbehrends rbehrends Nov 30, 2018

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.

string = CopyToStringRep(string);
MakeImmutableString(string);
}
Expand Down