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

HPCC-18886 Coverity issues in wssql component #10893

Merged

Conversation

Projects
None yet
5 participants
@rpastrana
Copy link
Member

commented Feb 20, 2018

  • Ensures dynamic_cast success before derefencing
  • Ensures member vars are initialized
  • Ensures null tested before accessing
  • Replaces NULL with nullptr

Signed-off-by: Rodrigo Pastrana rodrigo.pastrana@lexisnexis.com

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Testing:

@hpcc-jirabot

This comment has been minimized.

Copy link

commented Feb 20, 2018

@rpastrana

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

@kunalaswani these changes have been unit tested, but we need to perform a regression test. Let's run the JDBC based regression suite against this branch: rpastrana:HPCC-18886-coverityWssql. Thanks.

@rpastrana

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2018

@Michael-Gardner please review.

@Michael-Gardner
Copy link
Contributor

left a comment

Looks good other than one possible change I found, but its a style issue and not functional.

@@ -425,7 +425,7 @@ void myDisplayRecognitionError (pANTLR3_BASE_RECOGNIZER recognizer,pANTLR3_UINT8
// correct stream. Then we can see that the token we are looking at
// is just something that should not be there and throw this exception.
//
if (tokenNames == NULL)
if (tokenNames == nullptr)

This comment has been minimized.

Copy link
@Michael-Gardner

Michael-Gardner Feb 22, 2018

Contributor

A small question on style. Elsewhere we don't use brackets {} if the conditional only has one action afterwards. Should we remove the brackets here and just use indentation?

This comment has been minimized.

Copy link
@rpastrana

rpastrana Feb 22, 2018

Author Member

ummm good eye... in this case, I'm not sure it's worth changing. @Michael-Gardner

@Michael-Gardner

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2018

@rpastrana back to you.

@rpastrana

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2018

@richardkchapman please merge.

@@ -1089,10 +1089,13 @@ void CwssqlEx::createWUXMLParams(StringBuffer & xmlparams, const IArrayOf <ISQLE
if (exp->getExpType() == Value_ExpressionType)
{
SQLValueExpression * currentvalplaceholder = dynamic_cast<SQLValueExpression *>(exp);
currentvalplaceholder->trimTextQuotes();

This comment has been minimized.

Copy link
@richardkchapman

richardkchapman Feb 23, 2018

Member

In this and other cases, it seems to me that you are fixing the symptom (that Coverity complains your code is not safe, as a dynamic cast may return null) rather than the real bug. Yes, your change will suppress the warning, and yes, if the dynamic cast did return null then you will now skip some code rather than coring. But you've already checked the expression type, so if the dynamic cast DOES return null then it implies something has gone really wrong somewhere, and simply ignoring that fact and continuing silently is not really the correct behaviour.

Seems to me that the more correct fix (which would also improve efficiency - dynamic cast is very slow) is to change the dynamic cast to a static one.

This comment has been minimized.

Copy link
@rpastrana

rpastrana Feb 23, 2018

Author Member

In agreement, will revise

@richardkchapman

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

@rpastrana

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2018

@richardkchapman please review latest commit
de0ab45. Several optimizations based on your earlier comment.

@rpastrana

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2018

This branch passed regression test

@@ -500,7 +500,10 @@ void ECLEngine::generateSelectStruct(HPCCSQLTreeWalker * selectsqlobj, IProperti
}
else if (col->getExpType() == Function_ExpressionType)
{
SQLFunctionExpression * funcexp = dynamic_cast<SQLFunctionExpression *>(col);
SQLFunctionExpression * funcexp = static_cast<SQLFunctionExpression *>(col);
if (!funcexp)

This comment has been minimized.

Copy link
@richardkchapman

richardkchapman Mar 14, 2018

Member

This test is spurious (and coverity will complain about it). funcexp cannot be null following a static-cast unless col was null - and if col was null then you cored two lines ago.

@rpastrana rpastrana force-pushed the rpastrana:HPCC-18886-coverityWssql branch from de0ab45 to bb25671 Mar 14, 2018

@HPCCSmoketest

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

Automated Smoketest:
OS: centos 7.2.1511 (Linux 3.10.0-327.28.3.el7.x86_64)
Sha: bb25671
Build: success
Install hpccsystems-platform-community_6.5.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test total passed failed errors timeout
unittest 83 83 0 0 0
wutoolTest(Dali) 19 19 0 0 0
wutoolTest(Cassandra) 19 19 0 0 0

Regression test result:

phase total pass fail
setup (hthor) 11 11 0
setup (thor) 11 11 0
setup (roxie) 11 11 0
test (hthor) 765 765 0
test (thor) 679 679 0
test (roxie) 792 792 0

HPCC Stop: OK
HPCC Uninstall: OK
Time stats:

Prep time Build time Package time Install time Start time Test time Stop time Summary
7 sec (00:00:07) 182 sec (00:03:02) 55 sec (00:00:55) 6 sec (00:00:06) 37 sec (00:00:37) 1172 sec (00:19:32) 33 sec (00:00:33) 1492 sec (00:24:52)
@richardkchapman

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

Please squash

@richardkchapman

This comment has been minimized.

Copy link
Member

commented Mar 15, 2018

HPCC-18886 Coverity issues in wssql component
- Ensures dynamic_cast success before derefencing
- Ensures member vars are initialized
- Ensures null tested before accessing
- Replaces NULL with nullptr
- Removes unneeded casts
- Converts Dynamic to Static casts when appropriate
- Converts groupbyList/orderbyList to SQLFieldValueExpression arrays
- Rearranges toString logic to make use of new arrays
- Removes spurious static_cast result checks

Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexis.com>

@rpastrana rpastrana force-pushed the rpastrana:HPCC-18886-coverityWssql branch from bb25671 to bf8199b Mar 15, 2018

@rpastrana

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2018

@richardkchapman all commits squashed, ready for merge.

@richardkchapman richardkchapman merged commit bbbeea6 into hpcc-systems:master Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.