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

Sema: fix OOB access in coerceTupleToStruct #19620

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

wrongnull
Copy link
Contributor

Closes #19529

@wrongnull
Copy link
Contributor Author

not sure if I should add test for this

@Rexicon226
Copy link
Contributor

Probably should add a test yes :)

Copy link
Member

@jacobly0 jacobly0 left a comment

Choose a reason for hiding this comment

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

not sure if I should add test for this

The default answer to this question is always yes.

src/Sema.zig Outdated
Comment on lines 32337 to 32340
.struct_type => if (ip.loadStructType(inst_ty.toIntern()).field_names.len > 0)
ip.loadStructType(inst_ty.toIntern()).field_names.get(ip)[tuple_field_index]
else
try ip.getOrPutStringFmt(sema.gpa, "{d}", .{tuple_field_index}, .no_embedded_nulls),
Copy link
Member

@jacobly0 jacobly0 Apr 11, 2024

Choose a reason for hiding this comment

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

Suggested change
.struct_type => if (ip.loadStructType(inst_ty.toIntern()).field_names.len > 0)
ip.loadStructType(inst_ty.toIntern()).field_names.get(ip)[tuple_field_index]
else
try ip.getOrPutStringFmt(sema.gpa, "{d}", .{tuple_field_index}, .no_embedded_nulls),
.struct_type => ip.loadStructType(inst_ty.toIntern()).fieldName(ip, tuple_field_index).unwrap() orelse
try ip.getOrPutStringFmt(sema.gpa, "{d}", .{tuple_field_index}, .no_embedded_nulls),

Note that the same thing can be done in the .anon_struct_type case.

Copy link
Member

@jacobly0 jacobly0 Apr 11, 2024

Choose a reason for hiding this comment

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

Although, really it should probably be more like:

        const maybe_field_name = switch (ip.indexToKey(inst_ty.toIntern())) {
            .anon_struct_type => |anon_struct_type| anon_struct_type.fieldName(ip, tuple_field_index),
            .struct_type => ip.loadStructType(inst_ty.toIntern()).fieldName(ip, tuple_field_index),
            else => unreachable,
        };
        const struct_field_index = if (maybe_field_name.unwrap()) |field_name|
            try sema.structFieldIndex(block, struct_ty, field_name, field_src)
        else
            tuple_field_index;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@wrongnull wrongnull requested a review from jacobly0 April 12, 2024 07:44
Copy link
Member

@jacobly0 jacobly0 left a comment

Choose a reason for hiding this comment

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

I was trying to avoid the extra call to getOrPutStringFmt in the .anon_struct_type case, but the new code is certainly reasonable enough that I won't hold up the merge over it.

@wrongnull
Copy link
Contributor Author

Screenshot_20240603_214413

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coercing typed tuple to struct causes index OOB in Sema
3 participants