Skip to content

refactor(transformer/typescript): use a memory-safe implementation instead#3481

Merged
graphite-app[bot] merged 1 commit intomainfrom
05-31-refactor_transformer_typescript_use_a_memory-safe_implementation_instead
May 31, 2024
Merged

refactor(transformer/typescript): use a memory-safe implementation instead#3481
graphite-app[bot] merged 1 commit intomainfrom
05-31-refactor_transformer_typescript_use_a_memory-safe_implementation_instead

Conversation

@Dunqing
Copy link
Copy Markdown
Member

@Dunqing Dunqing commented May 31, 2024 •
edited
Loading

The previous implementation causes memory double free, but I don't know why.

@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app bot commented May 31, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@Dunqing Graphite App
Copy link
Copy Markdown
Member Author

Dunqing commented May 31, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Dunqing and the rest of your teammates on Graphite Graphite

This was referenced May 31, 2024
Merged
Merged
@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label May 31, 2024
@Dunqing Dunqing marked this pull request as ready for review May 31, 2024 05:41
@Dunqing
Copy link
Copy Markdown
Member Author

Dunqing commented May 31, 2024

Do you happen to know why that is? @overlookmotel

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented May 31, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #3481 will not alter performance

Comparing 05-31-refactor_transformer_typescript_use_a_memory-safe_implementation_instead (9dc58d5) with main (25e5bdd)

Summary

✅ 22 untouched benchmarks

@overlookmotel overlookmotel mentioned this pull request May 31, 2024
Closed
@overlookmotel
Copy link
Copy Markdown
Member

overlookmotel commented May 31, 2024 •
edited
Loading

@Dunqing I can't say for sure in this case, but I strongly suspect the culprit is AstBuilder::copy. I had been meaning to open an issue about the unsoundness of this API before, but hadn't got around to it - #3483.

What is odd is that all the objects being dealt with in this code are in the arena and don't have Drop impls, so nothing should get freed at all (much less double-freed). How did the double-free manifest? (error message? Miri found it?)

@Boshen Boshen force-pushed the 05-31-fix_transformer_typescript_if_this_statement_is_typescript_syntax_replace_it_with_a_blockstatement branch from 2ef6928 to 25e5bdd Compare May 31, 2024 07:24
@Boshen Boshen changed the base branch from 05-31-fix_transformer_typescript_if_this_statement_is_typescript_syntax_replace_it_with_a_blockstatement to main May 31, 2024 07:29
@Dunqing
Copy link
Copy Markdown
Member Author

Dunqing commented May 31, 2024 •
edited
Loading

What is odd is that all the objects being dealt with in this code are in the arena and don't have Drop impls, so nothing should get freed at all (much less double-freed). How did the double-free manifest? (error message? Miri found it?)

I got this warning after I changed assignments: Vec<'a, Statement<'a>> to assignments: std:vec::Vec<'a, Statement<'a>>. I did this because I found that when I pushed the statement to assignments, the statement would change

I print AST here.
image

First print

ExpressionStatement(
    ExpressionStatement {
        span: Span {
            start: 0,
            end: 0,
        },
        expression: AssignmentExpression(
            AssignmentExpression {
                span: Span {
                    start: 0,
                    end: 0,
                },
                operator: Assign,
                left: StaticMemberExpression(
                    StaticMemberExpression {
                        span: Span {
                            start: 0,
                            end: 0,
                        },
                        object: ThisExpression(
                            ThisExpression {
                                span: Span {
                                    start: 0,
                                    end: 0,
                                },
                            },
                        ),
                        property: IdentifierName {
                            span: Span {
                                start: 0,
                                end: 0,
                            },
                            name: "y",
                        },
                        optional: false,
                    },
                ),
                right: Identifier(
                    IdentifierReference {
                        span: Span {
                            start: 0,
                            end: 0,
                        },
                        name: "y",
                        reference_id: Cell {
                            value: None,
                        },
                        reference_flag: ReferenceFlag(
                            0x0,
                        ),
                    },
                ),
            },
        ),
    },
)

Second print

Some(
    ExpressionStatement(
        ExpressionStatement {
            span: Span {
                start: 0,
                end: 0,
            },
            expression: AssignmentExpression(
                AssignmentExpression {
                    span: Span {
                        start: 0,
                        end: 0,
                    },
                    operator: Assign,
                    left: StaticMemberExpression(
                        StaticMemberExpression {
                            span: Span {
                                start: 0,
                                end: 0,
                            },
                            object: ThisExpression(
                                ThisExpression {
                                    span: Span {
                                        start: 2541748224,
                                        end: 87808,
                                    },
                                },
                            ),
                            property: IdentifierName {
                                span: Span {
                                    start: 0,
                                    end: 0,
                                },
                                name: "\u{c}",
                            },
                            optional: false,
                        },
                    ),
                    right: Identifier(
                        IdentifierReference {
                            span: Span {
                                start: 0,
                                end: 0,
                            },
                            name: "y",
                            reference_id: Cell {
                                value: None,
                            },
                            reference_flag: ReferenceFlag(
                                0x0,
                            ),
                        },
                    ),
                },
            ),
        },
    ),
)

You will find that the left expression of these two results is different. This is very odd.

@Dunqing Dunqing force-pushed the 05-31-refactor_transformer_typescript_use_a_memory-safe_implementation_instead branch from 2161788 to 6ea2bb2 Compare May 31, 2024 07:39
@Dunqing Dunqing mentioned this pull request May 31, 2024
Closed
@overlookmotel
Copy link
Copy Markdown
Member

overlookmotel commented May 31, 2024

I can't claim to understand what exactly is causing the problem, but the data corruption looks like a use-after-free.

The change to std:vec::Vec would explain why the double-free warning appears - std:vec::Vec does implement Drop. I think this is just surfacing an existing problem - it was always UB, but the fact that none of the data gets dropped in the arena was hiding that until you switched to std:vec::Vec.

But whatever the exact cause, I strongly suspect AstBuilder::copy is the ultimate source of the problem. That API is completely unsound.

@Dunqing
Copy link
Copy Markdown
Member Author

Dunqing commented May 31, 2024

But whatever the exact cause, I strongly suspect AstBuilder::copy is the ultimate source of the problem. That API is completely unsound.

Yes! It's also very difficult to debug

@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app bot commented May 31, 2024 •
edited
Loading

Merge activity

@Dunqing
refactor(transformer/typescript): use a memory-safe implementation in…
9dc58d5 
…stead (#3481)

The previous implementation causes memory double free, but I don't know why.
@Dunqing Dunqing force-pushed the 05-31-refactor_transformer_typescript_use_a_memory-safe_implementation_instead branch from 6ea2bb2 to 9dc58d5 Compare May 31, 2024 11:15
@graphite-app graphite-app bot merged commit 9dc58d5 into main May 31, 2024
@graphite-app graphite-app bot deleted the 05-31-refactor_transformer_typescript_use_a_memory-safe_implementation_instead branch May 31, 2024 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

A-transformer Area - Transformer / Transpiler

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Dunqing @overlookmotel