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

Improvements to C code that can improvement robustness #455

Closed
nhasabni opened this issue Aug 9, 2021 · 5 comments · Fixed by #458
Closed

Improvements to C code that can improvement robustness #455

nhasabni opened this issue Aug 9, 2021 · 5 comments · Fixed by #458

Comments

@nhasabni
Copy link
Contributor

nhasabni commented Aug 9, 2021

Hi,

I work at Intel, and we have developed a tool that detects anomalous programming language expressions that can possibly lead to bugs. We scanned the code repository for this project as it has considerably high number of stars!

We found a couple of places where the code/expressions are confusing and seem to implement the logic in a rather convoluted manner. We think that the expressions could be rewritten to capture the logic accurately and precisely.

Case 1) libs/basekit/source/UArray.c:999

The expression if (UArray_greaterThan_(self, other) | UArray_equals_(self, other)) looks confusing because the intended logic of logical OR seems to be implemented using bitwise OR on integers. While this is technically correct, it is a bit confusing for someone reading this code for the first time. It looks like the better approach would be: 1) one option would be to use bool type from C99, and 2) another option would be to use enum type for true and false.

Case 2) extras/win32vc2005/freeglut-2.2.0/src/freeglut_structure.c#L624 and L626

It looks like expressions if( ln = (SFG_Node *)node->Next ) and if( ln = (SFG_Node *)node->Prev ) in line 624 and 626 resp., are missing parenthesis around the assignment. The expressions, similar to if( (node->Next = next) ), in other parts of the code correctly add those parenthesis.

Any thoughts on the findings? If this looks acceptable, I'm happy to send a pull request with the changes.

Thanks.

@stevedekorte
Copy link
Member

Hi Niranjan,

Thanks for finding these issues. Your tool sounds very useful! A pull request would be much appreciated.

Cheers,
Steve

@stevedekorte
Copy link
Member

Btw, what did you mean by "a considerably high number of stars"? :)

@nhasabni
Copy link
Contributor Author

nhasabni commented Aug 9, 2021

Hi Steve,

Thanks. Yes, I can submit a pull request.

Sorry my phrase was not precise. :) By stars, I meant the GitHub stars - we have analyzed more than 6000 repositories that use C as the primary language and have more than 100 GitHub stars, and 2.3K stars for this repository are higher than several other repositories.

@nhasabni
Copy link
Contributor Author

nhasabni commented Oct 1, 2021

hi Steve,

Sorry for the delay, just to update - I am working on sending a pull request. Please don't close this issue yet. :) Thanks

nhasabni added a commit to nhasabni/io that referenced this issue Oct 11, 2021
Rewrote UArray comparison functions to use boolean type instead of int type to
resolve any ambiguity. Modified signatures of the corresponding functions.

Ran tests and checked that they pass:
$ LD_LIBRARY_PATH=`pwd`/install/lib/ ../install/bin/io
../libs/iovm/tests/correctness/run.io
......................................................................
......................................................................
......................................................................
....................
----------------------------------------------------------------------
Ran 230 tests in 3.0030000000000001s

OK                                                                 run
@ales-tsurko ales-tsurko linked a pull request Oct 12, 2021 that will close this issue
@stevedekorte
Copy link
Member

Thanks for the pull request Niranjan. This is a nice improvement.

ales-tsurko added a commit that referenced this issue Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants