Modify ReturnStatement to prevent a UaF #76

pitust · 2020-11-27T15:21:24Z

This prevents a use after free and a valgrind error like this:

==671682== Memcheck, a memory error detector
==671682== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==671682== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==671682== Command: ./x
==671682== 
==671682== Invalid read of size 2
==671682==    at 0x1091BE: return_from_obj (in /home/pitust/code/pwg/x)
==671682==    by 0x1091D0: main (in /home/pitust/code/pwg/x)
==671682==  Address 0x4a6d040 is 0 bytes inside a block of size 2 free'd
==671682==    at 0x483B9AB: free (vg_replace_malloc.c:538)
==671682==    by 0x1091B9: return_from_obj (in /home/pitust/code/pwg/x)
==671682==    by 0x1091D0: main (in /home/pitust/code/pwg/x)
==671682==  Block was alloc'd at
==671682==    at 0x483A77F: malloc (vg_replace_malloc.c:307)
==671682==    by 0x10917A: return_from_obj (in /home/pitust/code/pwg/x)
==671682==    by 0x1091D0: main (in /home/pitust/code/pwg/x)
==671682== 
1
==671682== 
==671682== HEAP SUMMARY:
==671682==     in use at exit: 0 bytes in 0 blocks
==671682==   total heap usage: 2 allocs, 2 frees, 1,026 bytes allocated
==671682== 
==671682== All heap blocks were freed -- no leaks are possible
==671682== 
==671682== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
==671682== 
==671682== 1 errors in context 1 of 1:
==671682== Invalid read of size 2
==671682==    at 0x1091BE: return_from_obj (in /home/pitust/code/pwg/x)
==671682==    by 0x1091D0: main (in /home/pitust/code/pwg/x)
==671682==  Address 0x4a6d040 is 0 bytes inside a block of size 2 free'd
==671682==    at 0x483B9AB: free (vg_replace_malloc.c:538)
==671682==    by 0x1091B9: return_from_obj (in /home/pitust/code/pwg/x)
==671682==    by 0x1091D0: main (in /home/pitust/code/pwg/x)
==671682==  Block was alloc'd at
==671682==    at 0x483A77F: malloc (vg_replace_malloc.c:307)
==671682==    by 0x10917A: return_from_obj (in /home/pitust/code/pwg/x)
==671682==    by 0x1091D0: main (in /home/pitust/code/pwg/x)
==671682== 
==671682== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

This also adds a nodejs-only environment variable DEBUG which, when set, produces a wealth of debug output (aka backtraces on every template instantiation).
Fixes #74

Signed-off-by: pitust <[email protected]>

src/nodes/statements.ts

andrei-markeev · 2020-11-27T15:54:25Z

src/nodes/variable.ts

@@ -113,45 +113,43 @@ export class CVariableAllocation extends CTemplateBase {
 }

 @CodeTemplate(`
-{#statements}


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

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

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.

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

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.

src/template.ts

andrei-markeev · 2020-11-28T02:07:47Z

Sorry, but I cannot accept this PR in it's current form.
I really appreciate your contribution, but the motto of TS2C has been, from the beginning, "No excessive code that is not actually needed is ever generated.".
I am trying to stick to this rule as much as possible.
Even though C compiler will optimize out this temp variable, emitting it when it is not needed, is not acceptable.

Also, would be better to at least split out debugging stuff into a separate PR (or drop it altogether). You can of course keep it in your fork if it helps you. But I am not sure I can merge it before I fully understand why we need this extra condition in templating engine.

Signed-off-by: pitust <[email protected]>

pitust · 2020-12-13T12:54:18Z

@andrei-markeev i think that's it. It also fixes an internal bug in the templater (the prototype wasn't reassigned in the decorator).

andrei-markeev · 2020-12-17T23:36:57Z

hey! thanks for the update and sorry for the delay with response. NY time, quite hectic 😓

I found some more cases where this problem exists. Will investigate, and get back to you!

bil-ash · 2023-09-21T02:19:24Z

@andrei-markeev Are there any pending issues in this PR? If not, please merge this.

andrei-markeev · 2024-12-26T15:32:25Z

src/nodes/statements.ts

+        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() + ' ');


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.

andrei-markeev · 2024-12-26T15:40:14Z

src/nodes/statements.ts

+            // 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);


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

andrei-markeev · 2024-12-26T15:41:26Z

src/nodes/statements.ts

+        this.returnTemp = scope.root.symbolsHelper.addTemp(node, 'returnVal');
+        let returnType = scope.root.typeHelper.getCType(node.expression);


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

andrei-markeev · 2024-12-26T15:45:36Z

src/nodes/variable.ts

@@ -113,45 +113,43 @@ export class CVariableAllocation extends CTemplateBase {
 }

 @CodeTemplate(`
-{#statements}


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.

andrei-markeev · 2024-12-26T16:33:35Z

I experimented a bit more, and if we expand the test case for key to be non-primitive, then it doesn't work anymore:

function return_from_obj() {
    let obj = { key: { hello: "world" } };
    return obj.key;
}

console.log(return_from_obj());

this produces the following generated code:

struct return_from_obj_t {
    const char * hello;
};
struct obj_t {
    struct return_from_obj_t * key;
};

static struct return_from_obj_t * tmp_result;
struct return_from_obj_t * return_from_obj()
{
    struct obj_t * obj;
    struct return_from_obj_t * tmp_obj = NULL;
    struct return_from_obj_t * ret;

    tmp_obj = malloc(sizeof(*tmp_obj));
    assert(tmp_obj != NULL);
    tmp_obj->hello = "world";

    obj = malloc(sizeof(*obj));
    assert(obj != NULL);
    obj->key = tmp_obj;

    ret = obj->key;

    free(obj);
    free(tmp_obj); // <--- wrong, essentially frees `obj->key`
    return ret;
}

So the bigger issue here is that escape mechanism doesn't correctly detect escaping of subkeys via return statements.

I'll try to add it.

andrei-markeev · 2024-12-26T17:37:14Z

Escape analysis is fixed in 0bda2b3

pitust added 3 commits November 27, 2020 15:09

Fix andrei-markeev#74

a2b0289

Signed-off-by: pitust <[email protected]>

Move the tests to the correct directory

7a140b3

Signed-off-by: pitust <[email protected]>

Move the tests to the correct directory

2378b95

Signed-off-by: pitust <[email protected]>

andrei-markeev requested changes Nov 27, 2020

View reviewed changes

pitust requested a review from andrei-markeev November 27, 2020 17:10

pitust added 3 commits December 13, 2020 11:54

removed debug feature

e40d46c

remove unnecessary code generation

da64643

Signed-off-by: pitust <[email protected]>

Fix the warning and CReturnStatement.doCheckVarNeeded

e32b352

andrei-markeev requested changes Dec 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify ReturnStatement to prevent a UaF #76

Modify ReturnStatement to prevent a UaF #76

pitust commented Nov 27, 2020

andrei-markeev Nov 27, 2020

pitust Nov 27, 2020

andrei-markeev Dec 26, 2024 •

edited

Loading

andrei-markeev Dec 27, 2024

andrei-markeev Dec 27, 2024

andrei-markeev commented Nov 28, 2020 •

edited

Loading

pitust commented Dec 13, 2020

andrei-markeev commented Dec 17, 2020 •

edited

Loading

bil-ash commented Sep 21, 2023

andrei-markeev Dec 26, 2024

andrei-markeev Dec 26, 2024

andrei-markeev Dec 26, 2024 •

edited

Loading

andrei-markeev Dec 26, 2024 •

edited

Loading

andrei-markeev commented Dec 26, 2024

andrei-markeev commented Dec 26, 2024

		this.returnTemp = scope.root.symbolsHelper.addTemp(node, 'returnVal');
		let returnType = scope.root.typeHelper.getCType(node.expression);

Modify ReturnStatement to prevent a UaF #76

Are you sure you want to change the base?

Modify ReturnStatement to prevent a UaF #76

Conversation

pitust commented Nov 27, 2020

andrei-markeev Nov 27, 2020

Choose a reason for hiding this comment

pitust Nov 27, 2020

Choose a reason for hiding this comment

andrei-markeev Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

andrei-markeev Dec 27, 2024

Choose a reason for hiding this comment

andrei-markeev Dec 27, 2024

Choose a reason for hiding this comment

andrei-markeev commented Nov 28, 2020 • edited Loading

pitust commented Dec 13, 2020

andrei-markeev commented Dec 17, 2020 • edited Loading

bil-ash commented Sep 21, 2023

andrei-markeev Dec 26, 2024

Choose a reason for hiding this comment

andrei-markeev Dec 26, 2024

Choose a reason for hiding this comment

andrei-markeev Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

andrei-markeev Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

andrei-markeev commented Dec 26, 2024

andrei-markeev commented Dec 26, 2024

andrei-markeev Dec 26, 2024 •

edited

Loading

andrei-markeev commented Nov 28, 2020 •

edited

Loading

andrei-markeev commented Dec 17, 2020 •

edited

Loading

andrei-markeev Dec 26, 2024 •

edited

Loading

andrei-markeev Dec 26, 2024 •

edited

Loading