TGNumberEntry string length checks are inaccurate/dangerous. #17334
Assignees
Labels
Comments
|
Additional warnings by clang-tidy: |
I had a look at the code and I was left in awe by how abysmally it is written. Many of the conversion functions are redundant at best and plain wrong at worst, moreover they are written with no regards to efficiency (or memory safety) at all. Here is a taste of it (comments added by me): for (UInt_t i = 0; i < strlen(p); i++) { // recomputing strlen every time
if (isdigit(*p)) { // always testing the same character??
found++; // doesn't look like it's doing what it would like to do
}
}
while (found < digits) {
// coverity[secure_coding] // what??
// again, recomputing strlen every time
// also, unsafe function being used (with no bounds checking)
// also, this whole loop is redudant (why do 1 chr at a time?)
strcpy(p + strlen(p), "0");
found++;
}Is this code actually used? Can we deprecate and remove it? If not, we should really rewrite most of this stuff... |
2 tasks
|
I use this code actively for my GUI classes, please don't deprecate the class :). (Feel free though to remove useless parts within it) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
As seen in #16913, there is some attempt to guarantee that there will be no write past the end of buffer in the string manipulations. However in several places, it fall short (literally or maybe missing documentation).
We should consider replacing the fixed size buffer or improving the bound checks.
Namely the routines seems to assume that the buffer has a fixed length of 256 but in several place, the buffer is offset compare to its actual beginning.
StrInt(char *text, Long_t i, Int_t digits)hard-codes the length 250 for its input buffer, we should pass the actual length left there. In particular lineTGNumberEntry.cxx:310andTGNumberEntry.cxx:316needs to be updated.We should also review the rest
TGNumberEntry.cxxfor similar problematic patterns.The text was updated successfully, but these errors were encountered: