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

fix memleak in GetTokenFromStream #884

Merged
merged 1 commit into from
Oct 2, 2020
Merged

fix memleak in GetTokenFromStream #884

merged 1 commit into from
Oct 2, 2020

Conversation

ltx2018
Copy link
Contributor

@ltx2018 ltx2018 commented Jun 20, 2020

check asp & php if ParseAttribute return NULL

check asp & php if ParseAttribute return NULL
@geoffmcl
Copy link
Contributor

@ltx2018 thank you for the PR...

I have reviewed the commit 4377ab8, and it all looks fine... but...

Can you supply some small asp and php samples, to show the current memory leak... for testing the patch, to verify a fix... etc... etc... thanks...

@geoffmcl geoffmcl added the Bug label Sep 22, 2020
@ltx2018
Copy link
Contributor Author

ltx2018 commented Sep 30, 2020

@ltx2018 thank you for the PR...

I have reviewed the commit 4377ab8, and it all looks fine... but...

Can you supply some small asp and php samples, to show the current memory leak... for testing the patch, to verify a fix... etc... etc... thanks...

Thanks for the review.
Here is the reproduce testcode(based from oss-fuzz):
poc.zip.

While running under valgrind can get err like this:

==50584== Memcheck, a memory error detector
==50584== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==50584== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==50584== Command: ./poc
==50584==
==50584==
==50584== HEAP SUMMARY:
==50584==     in use at exit: 104 bytes in 1 blocks
==50584==   total heap usage: 483 allocs, 482 frees, 103,910 bytes allocated
==50584==
==50584== 104 bytes in 1 blocks are definitely lost in loss record 1 of 1
==50584==    at 0x4873BC0: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-arm64-linux.so)
==50584==    by 0x4917CFF: defaultAlloc (alloc.c:64)
==50584==    by 0x4907FEF: prvTidyNewNode (lexer.c:1430)
==50584==    by 0x49083FB: NewToken (lexer.c:1614)
==50584==    by 0x49083FB: ParsePhp (lexer.c:3561)
==50584==    by 0x49083FB: ParseAttribute (lexer.c:3616)
==50584==    by 0x490A29B: GetTokenFromStream (lexer.c:3261)
==50584==    by 0x490A29B: prvTidyGetToken (lexer.c:2506)
==50584==    by 0x4903C87: prvTidyParseDocument (parser.c:4606)
==50584==    by 0x491BB8B: prvTidyDocParseStream (tidylib.c:1488)
==50584==    by 0x491BE83: tidyDocParseBuffer (tidylib.c:1182)
==50584==    by 0x491BE83: tidyParseBuffer (tidylib.c:1109)
==50584==    by 0x400A13: run_tidy_parser (in /root/poc)
==50584==    by 0x400AAB: LLVMFuzzerTestOneInput (in /root/poc)
==50584==    by 0x400AF7: main (in /root/poc)
==50584==
==50584== LEAK SUMMARY:
==50584==    definitely lost: 104 bytes in 1 blocks
==50584==    indirectly lost: 0 bytes in 0 blocks
==50584==      possibly lost: 0 bytes in 0 blocks
==50584==    still reachable: 0 bytes in 0 blocks
==50584==         suppressed: 0 bytes in 0 blocks
==50584==
==50584== For counts of detected and suppressed errors, rerun with: -v
==50584== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

@geoffmcl
Copy link
Contributor

geoffmcl commented Oct 1, 2020

@ltx2018 thank you for the feedback... particularly your poc.c app, giving us a sample - in_884.xml -

<?xml
<?x

Built, in windows, where we do not have the valgrind tool, a memory debug version of the library... ran tidy.exe -q -xml in_884.xml and, in temptidy.txt output, note the allocation, and missing free of 104 bytes...

Applying commit 4377ab8, from your fork, this is certainly fixed... thanks...

Will get around to merging this PR soonest...

@geoffmcl geoffmcl merged commit ddbcd2a into htacg:next Oct 2, 2020
@geoffmcl
Copy link
Contributor

geoffmcl commented Oct 2, 2020

@ltx2018 thanks again... look forward to more...

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

Successfully merging this pull request may close these issues.

None yet

2 participants