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.

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

Modify ReturnStatement to prevent a UaF #76

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

Modify ReturnStatement to prevent a UaF #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a2b0289
Fix #74
pitust Nov 27, 2020
7a140b3
Move the tests to the correct directory
pitust Nov 27, 2020
2378b95
Move the tests to the correct directory
pitust Nov 27, 2020
e40d46c
removed debug feature
pitust Dec 13, 2020
da64643
remove unnecessary code generation
pitust Dec 13, 2020
e32b352
Fix the warning and CReturnStatement.doCheckVarNeeded
pitust Dec 13, 2020
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
40 changes: 35 additions & 5 deletions src/nodes/statements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as ts from 'typescript';
import { CodeTemplate, CodeTemplateFactory, CTemplateBase } from '../template';
import { CProgram, IScope} from '../program';
import { CProgram, IScope } from '../program';
import { ArrayType, NumberVarType, StringVarType } from '../types/ctypes';
import { CVariable, CVariableDeclaration, CVariableDestructors } from './variable';
import { CExpression, CCondition } from './expressions';
Expand Down Expand Up @@ -71,17 +71,48 @@ export class CEmptyStatement {
}

@CodeTemplate(`
{destructors}
return {expression};
{#if returnTypeAndVar}
{
{returnTypeAndVar} = {expression};
{destructors}
return {returnTemp};
}
{#else}
{destructors}
return {expression};
{/if}
`, ts.SyntaxKind.ReturnStatement)
export class CReturnStatement extends CTemplateBase {
public expression: CExpression;
public destructors: CVariableDestructors;
public retVarName: string = null;
public returnTypeAndVar: string = null;
public returnTemp: string = null;
public closureParams: { name: string, value: CExpression }[] = [];
doCheckVarNeeded(scope: IScope, node: ts.ReturnStatement): boolean {
let s = scope.root.memoryManager.getDestructorsForScope(node);
let result = false;
for (let e of s) {
result = result || new RegExp('\\W' + e.varName + '\\W').test(' ' + node.getFullText() + ' ');
Copy link
Owner

Choose a reason for hiding this comment

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

I am afraid relying on a text match here is too fragile and error-prone.

return statement can be huge, for example something like return function() { /* ... 200 lines of code here ... */ }, or something like return translate("Lorem ipsum dolor ... 5000 characters of random text here")... it can contain any imaginable text and it will not mean anything.

the right way would be to do something similar to what we do in memory.ts i.e. properly analyse the return statement and if a subkey of an object that was scheduled to be disposed in this scope escapes into the upper function, then we need to change the return statement to use the temporary variable.

}
return result;
}
constructor(scope: IScope, node: ts.ReturnStatement) {
super();
this.returnTemp = scope.root.symbolsHelper.addTemp(node, 'returnVal');
let returnType = scope.root.typeHelper.getCType(node.expression);
Comment on lines +102 to +103
Copy link
Owner

@andrei-markeev andrei-markeev Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's a good idea to move those two lines also into the if block because otherwise we're reserving the temp variable name even when it is not really needed

if (this.doCheckVarNeeded(scope, node)) {
// todo: make this less hackyy
let fakeVar = new CVariable(scope, '__fake', returnType, { removeStorageSpecifier: true, arraysToPointers: true });;
let fakeVarType = fakeVar.resolve().slice(0, -6).trim();
this.returnTypeAndVar = fakeVarType;
if (this.returnTypeAndVar.indexOf('{var}') == -1) {
this.returnTypeAndVar += ' {var}';
}
this.returnTypeAndVar = this.returnTypeAndVar.replace('{var}', this.returnTemp);
Comment on lines +105 to +112
Copy link
Owner

Choose a reason for hiding this comment

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

this is not only too hacky, it is also not a proper C89 as it produces a variable declaration that is not in the beginning of the block:

mem/mem7.c: In function ‘return_from_obj’:
mem/mem7.c:23:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
   23 |     struct return_from_obj_t * returnVal = obj->key;
      |     ^~~~~~

it's very easy to fix though, you just need to do smth like this:

if (tempVarIsNeeded) {
    this.retVarName = scope.root.symbolsHelper.addTemp(node, 'returnVal');
    let returnType = scope.root.typeHelper.getCType(node.expression);
    scope.variables.push(new CVariable(scope, this.retVarName, returnType, { removeStorageSpecifier: true }))
}

pushing the variable into the scope variables will automatically add it's declaration into the right place

}
this.expression = CodeTemplateFactory.createForNode(scope, node.expression);

this.destructors = new CVariableDestructors(scope, node);
}
}
Expand Down Expand Up @@ -380,8 +411,7 @@ export class CForInStatement extends CTemplateBase implements IScope {
else
this.init = new CElementAccess(scope, node.initializer);

if (node.statement.kind == ts.SyntaxKind.Block)
{
if (node.statement.kind == ts.SyntaxKind.Block) {
let block = <ts.Block>node.statement;
for (let s of block.statements)
this.statements.push(CodeTemplateFactory.createForNode(this, s));
Expand Down
76 changes: 37 additions & 39 deletions src/nodes/variable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,45 +113,43 @@ export class CVariableAllocation extends CTemplateBase {
}

@CodeTemplate(`
{#statements}
Copy link
Owner

Choose a reason for hiding this comment

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

I think {#statements} was there for a reason...

Copy link
Author

Choose a reason for hiding this comment

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

I don't really know, but i know the tests pass... I removed statements because it moved it above the actual return code.

Copy link
Owner

@andrei-markeev andrei-markeev Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tested it and basically the main side effect is that it inserts empty lines in random places if destructors are not within {#statements} block. so even though it's not critical, but I would say that in the best case scenario we probably want to keep {#statements}.

I think if {returnTypeAndVar} = {expression}; line is also put into {#statements} then it will appear above destructors.

Copy link
Owner

Choose a reason for hiding this comment

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

I think if {returnTypeAndVar} = {expression}; line is also put into {#statements} then it will appear above destructors.

my assumption was incorrect

I guess we will have to get rid of {#statements} after all, but it would still be great to fix the whitespaces somehow. I'm looking into it now

Copy link
Owner

Choose a reason for hiding this comment

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

General whitespace handling improvements were done in c27ad6d

However, I noticed there's another side effect of removing {#statements} from destructors: it adds destructors after return in some cases. It is an unreachable code, that's why tests weren't failing, but it's quite bad still, obviously.

I'll have to investigate and fix it.

{arrayDestructors => for (gc_i = 0; gc_i < ({this} ? {this}->size : 0); gc_i++) free((void*){this}->data[gc_i]);\n}
{destructors => free({this});\n}
{#if gcArraysCVarName}
for (gc_i = 0; gc_i < {gcArraysCVarName}->size; gc_i++) {
for (gc_j = 0; gc_j < ({gcArraysCVarName}->data[gc_i] ? {gcArraysCVarName}->data[gc_i]->size : 0); gc_j++)
free((void*){gcArraysCVarName}->data[gc_i]->data[gc_j]);\n
free({gcArraysCVarName}->data[gc_i] ? {gcArraysCVarName}->data[gc_i]->data : NULL);
free({gcArraysCVarName}->data[gc_i]);
}
free({gcArraysCVarName}->data);
free({gcArraysCVarName});
{/if}
{#if gcArraysVarName}
for (gc_i = 0; gc_i < {gcArraysVarName}->size; gc_i++) {
free({gcArraysVarName}->data[gc_i]->data);
free({gcArraysVarName}->data[gc_i]);
}
free({gcArraysVarName}->data);
free({gcArraysVarName});
{/if}
{#if gcDictsVarName}
for (gc_i = 0; gc_i < {gcDictsVarName}->size; gc_i++) {
free({gcDictsVarName}->data[gc_i]->index->data);
free({gcDictsVarName}->data[gc_i]->index);
free({gcDictsVarName}->data[gc_i]->values->data);
free({gcDictsVarName}->data[gc_i]->values);
free({gcDictsVarName}->data[gc_i]);
}
free({gcDictsVarName}->data);
free({gcDictsVarName});
{/if}
{#if gcVarName}
for (gc_i = 0; gc_i < {gcVarName}->size; gc_i++)
free({gcVarName}->data[gc_i]);
free({gcVarName}->data);
free({gcVarName});
{/if}
{/statements}`
{arrayDestructors => for (gc_i = 0; gc_i < ({this} ? {this}->size : 0); gc_i++) free((void*){this}->data[gc_i]);\n}
{destructors => free({this});\n}
{#if gcArraysCVarName}
for (gc_i = 0; gc_i < {gcArraysCVarName}->size; gc_i++) {
for (gc_j = 0; gc_j < ({gcArraysCVarName}->data[gc_i] ? {gcArraysCVarName}->data[gc_i]->size : 0); gc_j++)
free((void*){gcArraysCVarName}->data[gc_i]->data[gc_j]);\n
free({gcArraysCVarName}->data[gc_i] ? {gcArraysCVarName}->data[gc_i]->data : NULL);
free({gcArraysCVarName}->data[gc_i]);
}
free({gcArraysCVarName}->data);
free({gcArraysCVarName});
{/if}
{#if gcArraysVarName}
for (gc_i = 0; gc_i < {gcArraysVarName}->size; gc_i++) {
free({gcArraysVarName}->data[gc_i]->data);
free({gcArraysVarName}->data[gc_i]);
}
free({gcArraysVarName}->data);
free({gcArraysVarName});
{/if}
{#if gcDictsVarName}
for (gc_i = 0; gc_i < {gcDictsVarName}->size; gc_i++) {
free({gcDictsVarName}->data[gc_i]->index->data);
free({gcDictsVarName}->data[gc_i]->index);
free({gcDictsVarName}->data[gc_i]->values->data);
free({gcDictsVarName}->data[gc_i]->values);
free({gcDictsVarName}->data[gc_i]);
}
free({gcDictsVarName}->data);
free({gcDictsVarName});
{/if}
{#if gcVarName}
for (gc_i = 0; gc_i < {gcVarName}->size; gc_i++)
free({gcVarName}->data[gc_i]);
free({gcVarName}->data);
free({gcVarName});
{/if}`
)
export class CVariableDestructors extends CTemplateBase {
public gcVarName: string = null;
Expand Down
14 changes: 8 additions & 6 deletions src/template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {IScope} from './program';
import { IScope } from './program';

interface INode { kind: number, getText(): string };

Expand All @@ -16,7 +16,7 @@ export class CodeTemplateFactory {
: "/* Unsupported node: " + node.getText().replace(/[\n\s]+/g, ' ') + " */;\n";
}
public static templateToString(template: string | CTemplateBase) {
return typeof(template) === "string" ? template : template.resolve();
return typeof (template) === "string" ? template : template.resolve();
}
}

Expand All @@ -42,6 +42,8 @@ export function CodeTemplate(tempString: string, nodeKind?: number | number[]):
nodeKindTemplates[nk] = newConstructor;
}

newConstructor.prototype = target.prototype;

return newConstructor;

} as ClassDecorator;
Expand Down Expand Up @@ -123,15 +125,15 @@ function processTemplate(template: string, args: string | CTemplateBase): [strin
let index = -1;
while ((index = template.indexOf("{" + k + "}")) > -1) {
let spaces = '';
while (template.length > index && template[index-1] == ' ') {
while (template.length > index && template[index - 1] == ' ') {
index--;
spaces+=' ';
spaces += ' ';
}
let value = args[k];
if (value && value.resolve)
value = value.resolve();
if (value && typeof value === 'string')
value = value.replace(/\n/g, '\n'+spaces);
value = value.replace(/\n/g, '\n' + spaces);
template = template.replace("{" + k + "}", () => value);
replaced = true;
}
Expand Down Expand Up @@ -226,7 +228,7 @@ function replaceArray(data, k, array, statements) {
if (elementsResolved != "")
elementsResolved += separator;
elementsResolved += resolvedElement;
}
}

}

Expand Down
1 change: 1 addition & 0 deletions tests/regression/regression1.res.expect
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1
7 changes: 7 additions & 0 deletions tests/regression/regression1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// regression test for #74
function return_from_obj() {
let obj = { key: 1 };
return obj.key;
}

console.log(return_from_obj());