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

Jump to bottom

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

wants to merge 6 commits into from

Conversation

pitust
Copy link

@pitust pitust commented Nov 27, 2020

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

@pitust
Fix andrei-markeev#74
a2b0289 
Signed-off-by: pitust <piotr@stelmaszek.com>
@pitust
Move the tests to the correct directory
7a140b3 
Signed-off-by: pitust <piotr@stelmaszek.com>
@pitust
Move the tests to the correct directory
2378b95 
Signed-off-by: pitust <piotr@stelmaszek.com>
andrei-markeev
andrei-markeev requested changes Nov 27, 2020
View reviewed changes
src/nodes/statements.ts Outdated Show resolved Hide resolved
src/nodes/variable.ts
@@ -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.

src/template.ts Outdated Show resolved Hide resolved
@andrei-markeev
Copy link
Owner

andrei-markeev commented Nov 28, 2020 •
edited
Loading

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.

@pitust
Copy link
Author

pitust commented Dec 13, 2020

@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
Copy link
Owner

andrei-markeev commented Dec 17, 2020 •
edited
Loading

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
Copy link

bil-ash commented Sep 21, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrei-markeev andrei-markeev Awaiting requested review from andrei-markeev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory allocation bug
3 participants
@pitust @andrei-markeev @bil-ash