avoid some unchecked pointer dereferences#4811
avoid some unchecked pointer dereferences#4811danmar merged 2 commits intocppcheck-opensource:mainfrom
Conversation
|
I filed https://trac.cppcheck.net/ticket/11566 about detecting this. |
| const ValueFlow::Value *value = getBufferSizeValue(array); | ||
| if (!value) | ||
| return false; | ||
| if (path) |
There was a problem hiding this comment.
I removed this check since it is always provided and all other output parameters were unchecked.
|
|
||
| bool CheckBufferOverrun::isCtuUnsafeBufferUsage(const Check *check, const Token *argtok, MathLib::bigint *offset, int type) | ||
| { | ||
| if (!offset) |
There was a problem hiding this comment.
Moved this check here since it would do all the work and then bailout if no output parameter was provided.
| const std::map<std::string, nonneg int>& map(bool global) const { | ||
| return global ? mVariableId_global : mVariableId; | ||
| } | ||
| nonneg int* getVarId() const { |
There was a problem hiding this comment.
This was a constness violation
firewave
force-pushed
the
ptr-ref
branch
2 times, most recently
from
04925cd to
9535ace
Compare
|
I filed the following tickets about the hidden |
| } | ||
|
|
||
| static bool addIncludePathsToList(const std::string& fileList, std::list<std::string>* pathNames) | ||
| static bool addIncludePathsToList(const std::string& fileList, std::list<std::string>& pathNames) |
There was a problem hiding this comment.
this is my programming style.
If you call addIncludePathsToList(fileList, pathNames) .. both parameters by value (you can not see it's passed by reference looking at the call).. then that means the addIncludePathsToList will not modify pathNames.
Since addIncludePathsToList modifies pathNames I think it's more explicit to write addIncludePathsToList(fileList, &pathNames). You can see in the function call it might modify pathNames.
There was a problem hiding this comment.
This is not a coding style decision. This is about safety and proper C++. If the parameter is not optional you might pass a nullptr (which we do but fortunately only is like three cases) and it will be dereferenced.
If this would be our style than we would have to convert every modifying reference to a pointer and also add checking of the pointer to each and every call. That would be a mess and would probably also hurt performance in a big way.
There was a problem hiding this comment.
Thinking about it a bit more this style doesn't really makes sense since you will only see this on the very first call and not subsequent ones in the chain.
|
@chrchr-github @pfultz2 @orbitcowboy We are supposed to be an application which enhances quality and security of code. And with having certain hangups about we treat our code we might actually end with the worst code base of all involved in some projects. This might sound quite harsh but we have been lucky that these have only caused us minor or impossible issues which could have been resolved with next to no work so far which could have been much worse. I am not the biggest fan of modern C++ (many things just fall apart if you try to use them) but if there are barebones things in C++/tooling which allow us to avoid certain errors we should leverage those. After all that is basically what we do the work in this project for. |
|
I'm all for avoiding pointers unless there is a specific need for them. |
|
Its good to use references where possible but its also useful to see at the call site which variables are modified as well. Perhaps we could use the nonull attribute, instead of a reference in those cases? |
|
That attribute would then essentially end up everywhere since there's usually one entrypoint very high up in case of things like The cases where you would actually see the "in" parameter in the call is only a very small part of the code so there is not much upside to that. And it is mostly variables that are declared directly in front of the call. (I actually have already gotten rid of most of all unchecked pointer usages in my local tree). The code is actually quite clean. So far I have come across only three(!) instances in out code where it could have actually happened. One in currently unreachable code, two were cases where the code in question just wasn't called in that context. And if we are doing this then it has to be consistent. No more |
|
well.. I think we can change to references since the community is doing this. As far as I remember.. the google c++ style guide used to advocate pointers for output parameters many years ago but I guess some very clever guys determined that it was best to drop that advice. |
Peer pressure is usually a bad argument. But it's also what I have been doing advocating for this. I try to go with the "common sense" choice but "my sense" might not be common. Sorry for repeating myself and being so blunt in this. Update: It's rather consistency. I do get why you want to do that and it makes sense in terms of readability but as I pointed out before it is hard to do consistently and it might not even show up. You could always deref the pointer at the beginning of the function so you never work on them and always have references and then again pass pointers but that seems like one of the worst pattern you could code. As it is a safety concern and I think as a security focused tool we should try to write code which assists us in that. Newer standards had you more safe containers and with those we also would lose the indicator if something is being written to.
Their style guide also includes "never use exceptions" which is kind of crazy - but in performance critical code that's understandable because of the overhead. They are definitely doing things their own way. |
4 participants
This also exposed a constness violation in
VariableMap.