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

tao_idl crashed with a wstring literal #984

Merged

Conversation

@wkbrd
Copy link
Contributor

wkbrd commented Oct 31, 2019

Switch shallow copy to a deep copy.

@iguessthislldo iguessthislldo self-requested a review Nov 1, 2019
Copy link
Member

iguessthislldo left a comment

Confirming this is a bug:

Given IDL like:

const wstring wstring_constant = L"日本語がわかリません。";

Then:

% bash tao_idl.sh --syntax-only wstring_literal.idl
processing wstring_literal.idl
free(): invalid pointer
tao_idl.sh: line 13:  9690 Aborted                 (core dumped) tao_idl $@

And this does fix that, but:

  1. This should put in the tests. I was going to suggest adding a simple test somewhere in TAO/tests/IDL_Test, but it appears it was tested at one point:

// const wstring wstr = L"wstr";

This is something I wish I would have known about before, because this is dumb, especially to comment them out without an explanation. There are a few more at least in this file, can you uncomment them?

  1. This introduces a memory leak into parsing the literals, because AST_Expression was taking ownership of the memory before. I think only wide string part in these lines have to be changed to delete the string that is being copied after create_expr:

ACE_TAO/TAO/TAO_IDL/fe/idl.ypp

Lines 2187 to 2197 in d7cbc42

| IDL_STRING_LITERAL
{
$$ = idl_global->gen ()->create_expr ($1);
$1->destroy ();
delete $1;
$1 = 0;
}
| IDL_WSTRING_LITERAL
{
$$ = idl_global->gen ()->create_expr ($1);
}

Changing this requires being able to regenerate the IDL parser using bison and flex with TAO/TAO_IDL/regen.pl.

@wkbrd

This comment has been minimized.

Copy link
Contributor Author

wkbrd commented Nov 1, 2019

Change request 1 has been implemented. Planning to investigate 2 later.

@iguessthislldo

This comment has been minimized.

Copy link
Member

iguessthislldo commented Nov 1, 2019

Planning to investigate 2 later.

I can do it for you and push it to your branch if you wish. If you want to do it yourself, that's fine too, just make sure you're using a fairly recent version of bison.

@wkbrd

This comment has been minimized.

Copy link
Contributor Author

wkbrd commented Nov 1, 2019

@iguessthislldo , if you are able to investigate and update 2, that would be helpful. Let me know if you need my help further. Thanks!

@iguessthislldo

This comment has been minimized.

Copy link
Member

iguessthislldo commented Nov 1, 2019

Alright so I was a little confused at first because I did what I suggested, but valgrind was saying my free was off by 2 bytes and there was still a memory leak:

==16321== Invalid free() / delete / delete[] / realloc()
==16321==    at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16321==    by 0x5895F82: ACE_OS::free(void*) (OS_NS_stdlib.cpp:77)
==16321==    by 0x54DA53F: tao_yyparse() (idl.ypp:2198)
==16321==    by 0x54CD685: FE_yyparse() (fe_extern.cpp:93)
==16321==    by 0x11515F: DRV_drive(char const*) (tao_idl.cpp:231)
==16321==    by 0x115A53: main (tao_idl.cpp:429)
==16321==  Address 0x6f38632 is 2 bytes inside a block of size 8 alloc'd
==16321==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==16321==    by 0x63C79B9: strdup (strdup.c:42)
==16321==    by 0x54EFC11: ACE_OS::strdup(char const*) (OS_NS_string.inl:219)
==16321==    by 0x54EB551: tao_yylex() (idl.ll:305)
==16321==    by 0x54D5B45: tao_yyparse() (idl.tab.cpp:2288)
==16321==    by 0x54CD685: FE_yyparse() (fe_extern.cpp:93)
==16321==    by 0x11515F: DRV_drive(char const*) (tao_idl.cpp:231)
==16321==    by 0x115A53: main (tao_idl.cpp:429)

But it turns out it is getting offset by two when the literal is being processed:

tao_yylval.wsval = idl_wstring_escape_reader (tmp + 2);

I've fixed that by returning a copy of the offset string and deleting the original. I will push my changes shortly.

If desired, this options keeps gperf from printing the whole path of the
gperf executable in the result. It only prints the arguments.
This also fixes the "invalid" part of the orginal invalid free issue in
#984
@iguessthislldo

This comment has been minimized.

Copy link
Member

iguessthislldo commented Nov 1, 2019

@mitza-oci I went ahead and made the change you wanted in gperf, adding an option to skip argv[0] when printing the arguments.

/* Command-line: -M -J -c -C -D -E -T -f 0 -a -o -t -p -K keyword_ -L C++ -Z TAO_IDL_CPP_Keyword_Table -N lookup -k1,2,$ --only-record-arguments fe/keywords.dat */
@wkbrd wkbrd requested a review from iguessthislldo Nov 1, 2019
@iguessthislldo iguessthislldo requested a review from mitza-oci Nov 4, 2019
ACE/apps/gperf/src/Options.cpp Outdated Show resolved Hide resolved
TAO/TAO_IDL/fe/idl.ypp Show resolved Hide resolved
…h_wstring_literals
@iguessthislldo iguessthislldo requested a review from mitza-oci Nov 8, 2019
ACE/apps/gperf/src/Options.cpp Outdated Show resolved Hide resolved
ACE/apps/gperf/src/Options.cpp Outdated Show resolved Hide resolved
ACE/apps/gperf/src/Options.cpp Outdated Show resolved Hide resolved
@mitza-oci

This comment has been minimized.

Copy link
Member

mitza-oci commented Nov 8, 2019

Something broke in your latest push, but before that it looks like a few of the Azure CI builds failed because the llvm apt repo was not configured (apt-add-repository step didn't run).

Copy link
Member

mitza-oci left a comment

Once it compiles OK on CI builds i should be ready to go.

@@ -434,10 +436,9 @@ Options::parse_args (int argc, ACE_TCHAR *argv[])
"-v\tPrints out the current version number and exits with a value of 0\n"
"-V\tExits silently with a value of 0.\n"
"-Z\tAllow user to specify name of generated C++ class. Default\n"
"\tname is `Perfect_Hash.'\n",
"\tname is `Perfect_Hash.'\n"

This comment has been minimized.

Copy link
@iguessthislldo

iguessthislldo Nov 8, 2019

Member

@mitza-oci I forgot to put the comma back 😅

@iguessthislldo iguessthislldo self-requested a review Nov 8, 2019
@iguessthislldo

This comment has been minimized.

Copy link
Member

iguessthislldo commented Nov 8, 2019

@wkbrd Thanks for bringing up the issue!

@iguessthislldo iguessthislldo merged commit c976707 into DOCGroup:master Nov 8, 2019
23 of 26 checks passed
23 of 26 checks passed
build
Details
DOCGroup.ACE_TAO Build #20191108.2 failed
Details
DOCGroup.ACE_TAO (Linux CLANG7) Linux CLANG7 failed
Details
DOCGroup.ACE_TAO (Linux CLANG8) Linux CLANG8 failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
CodeFactor No issues found.
Details
DOCGroup.ACE_TAO (Linux CLANG5) Linux CLANG5 succeeded
Details
DOCGroup.ACE_TAO (Linux CLANG6) Linux CLANG6 succeeded
Details
DOCGroup.ACE_TAO (Linux CLANG9) Linux CLANG9 succeeded
Details
DOCGroup.ACE_TAO (Linux GCC48) Linux GCC48 succeeded
Details
DOCGroup.ACE_TAO (Linux GCC6) Linux GCC6 succeeded
Details
DOCGroup.ACE_TAO (Linux GCC7) Linux GCC7 succeeded
Details
DOCGroup.ACE_TAO (Linux GCC8) Linux GCC8 succeeded
Details
DOCGroup.ACE_TAO (Linux GCC9) Linux GCC9 succeeded
Details
DOCGroup.ACE_TAO (MacOSX) MacOSX succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2015 Debug64) VisualStudio2015 Debug64 succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2015 Release64) VisualStudio2015 Release64 succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2017 Debug64) VisualStudio2017 Debug64 succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2017 Release64) VisualStudio2017 Release64 succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2017 WChar) VisualStudio2017 WChar succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2019 Debug32) VisualStudio2019 Debug32 succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2019 Debug64) VisualStudio2019 Debug64 succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2019 Release32) VisualStudio2019 Release32 succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2019 Release64) VisualStudio2019 Release64 succeeded
Details
DOCGroup.ACE_TAO (VisualStudio2019 WChar) VisualStudio2019 WChar succeeded
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.