8 ms·

LibreOffice Project's Check

86 points by mmastrac 12y ago

cremno 12y ago
>So do implement it then to avoid the compilation and/or linking errors, but make it crash intentionally if called. void SimpleReferenceObject::operator delete[](void * /* pPtr */) { free(NULL); } What will cause the crash? Passing a null pointer to free() is fine. Something specific to C++?
- Someone 12y ago
  I also think that code will run fine. The comment in LibreOffice's source and this blog post are incorrect. I also wonder how far PVS-Studio goes in deciding that 'expression is always true' in this fragment: if( (pSymDef->GetType() != SbxEMPTY) || (pSymDef->GetType() != SbxNULL) ) Is ::GetType() marked as pure in the LibreOffice source code, does PVS-Studio check the source code of its implementation(s), does PVS-Studio have heuristics for function names that likely are pure, or does it just assume any function is pure?
  - AndreyKarpov 12y ago
    The GetType() is const function for simple return type: class SbiSymDef { .... SbxDataType GetType() const { return eType; } .... };
- cesarb 12y ago
  It seems to have been introduced at the following commit: http://cgit.freedesktop.org/libreoffice/core/commit/?id=f3d96e6a3510b38b8c432b8d195d7aa2d2c0dc11 http://cgit.freedesktop.org/libreoffice/core/commit/?id=f3d9... I think even MSVC2008 should accept free(NULL) without crashing. My guess is that this was a case of tweaking the source code until the buggy compiler accepted it (it's within a "#ifdef _MSC_VER" block, so it's a MSVC-only workaround). He probably originally wrote the function containing something like std::abort(), but the compiler rejected it. So he changed it to a dummy (do-nothing) call to free(), and missed updating the comment he had written in the previous attempt.
- tines 12y ago
  I wonder what they thought was wrong with `abort`.
- EpicEng 12y ago
  Per the spec, free(NULL) is fine and well defined, essentially a no-op. Not sure what they're thinking. abort would have been an option.... And hell, were they trying to invoke UB and assuming that would cause a crash? Wrong on multiple levels.
  - pgeorgi 12y ago
    Spec is fine and all, but free(NULL) reliably crashes on Solaris and probably other systems.
    - cremno 12y ago
      The malloc(3C) man pages since 5.5.1 claim otherwise.
- AndreyKarpov 12y ago
  Call free(NULL); does not lead to any trouble. However, if you say that regularly use such statmen in your code, I'll be surprised. Analyzer surprised too. PVS suspects that it is not okay, and maybe we're dealing with some kind of typo or something else.
  - cremno 12y ago
    Yes, free(NULL) is a questionable statement. The source code comment (not the analyzer) just irritated me.
KNoureen 12y ago
Are these issues reported to LO devs, or they have to look it up themselves?
- AndreyKarpov 12y ago
  Readers' FAQ on Articles about PVS-Studio and CppCat: http://www.viva64.com/en/a/0085/ http://www.viva64.com/en/a/0085/
  - mintplant 12y ago
    Have you reported the bugs to the project developers? Have you sent them a patch? When checking open-source projects, we almost always report the results to their authors. If we publish such a report as an article, we try to send the link to it to the developers. If there are too few bugs in a project to write an article about, we still report whatever results we've got to the authors. Or rather, we try to - sometimes developers don't (strange as it may sound) have any contacts, or their bug trackers don't accept messages, or you need to enter a captcha which no one can solve. That's why we never send patches. There are a few reasons for that: 1. We are not familiar with the code and therefore cannot be sure if all the bugs we catch are really bugs. To understand that, we would need to study the project very closely. 2. Even with obvious bugs, we often can't say for sure how to fix them. 3. Finally, we pursue but one goal with our articles - to demonstrate the capabilities of the analyzer we develop. That is, we want to prove that our tool can find bugs in a real-life, living code. We don't aim at fixing bugs - we aim at proving that our tool can find them.
GFK_of_xmaspast 12y ago
Could the first example, with "getColor() == getColor()", in general, be a false positive if getColor() has side effects? bool getColor() { static a = 0; static b = 0; a++; b++; a %= 3; b %= 2; return (a && b); } or does something in the standard prevent this?
- jfeser 12y ago
  It could definitely be a false positive. That said, that would be pretty strange behavior for a function called getColor(). I'd expect that to return a property value, not modify some state.
- deleted 12y ago
  [deleted]
- EpicEng 12y ago
  Perhaps there is some semantic analysis involved (I don't know what tool they're using), but I have to imagine that getColor() is defined along the lines: T getColor() const; Otherwise that warning would often be at risk of emitting FP's, to the point at which it would become far less useful. Of course, even then... T getColor() { return rand(); // gotcha! }
- anuragbiyani 12y ago
  It's perfectly valid for the function getColor() to return different value for each invocation, e.g., think std::rand(). However, the function as you wrote, when used in the expression (getColor() == getColor()), results in undefined behavior. This is because equality operator (==) is not a sequence point in C++, and thus by calling getColor() twice, you are modifying variables a & b more than once between a pair of sequence points (see "Undefined behavior" section here: http://en.cppreference.com/w/cpp/language/eval_order http://en.cppreference.com/w/cpp/language/eval_order). In general, you cannot be sure of the order of evaluation for a statement of form say, f() == g(). The relative order in which f & g are called is unspecified. Also, the behavior can easily become undefined if you violate certain conditions in f & g (as in the case above).
  - 12y ago
twotwotwo 12y ago
These are especially interesting to me for the rules--it is not crazy hard to add things to Go's `go vet` tool (or write other checks outside vet with `go/ast`, etc.), and I wonder if "assigned variable twice consecutively" or "foo() == foo() looks suspicious" are good candidates.

86 points by mmastrac 12y ago

>So do implement it then to avoid the compilation and/or linking errors, but make it crash intentionally if called. void SimpleReferenceObject::operator delete[](void * /* pPtr */) { free(NULL); } What will cause the crash? Passing a null pointer to free() is fine. Something specific to C++?

Someone 12y ago
I also think that code will run fine. The comment in LibreOffice's source and this blog post are incorrect. I also wonder how far PVS-Studio goes in deciding that 'expression is always true' in this fragment: if( (pSymDef->GetType() != SbxEMPTY) || (pSymDef->GetType() != SbxNULL) ) Is ::GetType() marked as pure in the LibreOffice source code, does PVS-Studio check the source code of its implementation(s), does PVS-Studio have heuristics for function names that likely are pure, or does it just assume any function is pure?
- AndreyKarpov 12y ago
  The GetType() is const function for simple return type: class SbiSymDef { .... SbxDataType GetType() const { return eType; } .... };
cesarb 12y ago
It seems to have been introduced at the following commit: http://cgit.freedesktop.org/libreoffice/core/commit/?id=f3d96e6a3510b38b8c432b8d195d7aa2d2c0dc11 http://cgit.freedesktop.org/libreoffice/core/commit/?id=f3d9... I think even MSVC2008 should accept free(NULL) without crashing. My guess is that this was a case of tweaking the source code until the buggy compiler accepted it (it's within a "#ifdef _MSC_VER" block, so it's a MSVC-only workaround). He probably originally wrote the function containing something like std::abort(), but the compiler rejected it. So he changed it to a dummy (do-nothing) call to free(), and missed updating the comment he had written in the previous attempt.
tines 12y ago
I wonder what they thought was wrong with `abort`.
EpicEng 12y ago
Per the spec, free(NULL) is fine and well defined, essentially a no-op. Not sure what they're thinking. abort would have been an option.... And hell, were they trying to invoke UB and assuming that would cause a crash? Wrong on multiple levels.
- pgeorgi 12y ago
  Spec is fine and all, but free(NULL) reliably crashes on Solaris and probably other systems.
  - cremno 12y ago
    The malloc(3C) man pages since 5.5.1 claim otherwise.
AndreyKarpov 12y ago
Call free(NULL); does not lead to any trouble. However, if you say that regularly use such statmen in your code, I'll be surprised. Analyzer surprised too. PVS suspects that it is not okay, and maybe we're dealing with some kind of typo or something else.
- cremno 12y ago
  Yes, free(NULL) is a questionable statement. The source code comment (not the analyzer) just irritated me.

KNoureen 12y ago

Are these issues reported to LO devs, or they have to look it up themselves?

AndreyKarpov 12y ago
Readers' FAQ on Articles about PVS-Studio and CppCat: http://www.viva64.com/en/a/0085/ http://www.viva64.com/en/a/0085/
- mintplant 12y ago
  Have you reported the bugs to the project developers? Have you sent them a patch? When checking open-source projects, we almost always report the results to their authors. If we publish such a report as an article, we try to send the link to it to the developers. If there are too few bugs in a project to write an article about, we still report whatever results we've got to the authors. Or rather, we try to - sometimes developers don't (strange as it may sound) have any contacts, or their bug trackers don't accept messages, or you need to enter a captcha which no one can solve. That's why we never send patches. There are a few reasons for that: 1. We are not familiar with the code and therefore cannot be sure if all the bugs we catch are really bugs. To understand that, we would need to study the project very closely. 2. Even with obvious bugs, we often can't say for sure how to fix them. 3. Finally, we pursue but one goal with our articles - to demonstrate the capabilities of the analyzer we develop. That is, we want to prove that our tool can find bugs in a real-life, living code. We don't aim at fixing bugs - we aim at proving that our tool can find them.

GFK_of_xmaspast 12y ago

Could the first example, with "getColor() == getColor()", in general, be a false positive if getColor() has side effects? bool getColor() { static a = 0; static b = 0; a++; b++; a %= 3; b %= 2; return (a && b); } or does something in the standard prevent this?

jfeser 12y ago
It could definitely be a false positive. That said, that would be pretty strange behavior for a function called getColor(). I'd expect that to return a property value, not modify some state.
deleted 12y ago
[deleted]
EpicEng 12y ago
Perhaps there is some semantic analysis involved (I don't know what tool they're using), but I have to imagine that getColor() is defined along the lines: T getColor() const; Otherwise that warning would often be at risk of emitting FP's, to the point at which it would become far less useful. Of course, even then... T getColor() { return rand(); // gotcha! }
anuragbiyani 12y ago
It's perfectly valid for the function getColor() to return different value for each invocation, e.g., think std::rand(). However, the function as you wrote, when used in the expression (getColor() == getColor()), results in undefined behavior. This is because equality operator (==) is not a sequence point in C++, and thus by calling getColor() twice, you are modifying variables a & b more than once between a pair of sequence points (see "Undefined behavior" section here: http://en.cppreference.com/w/cpp/language/eval_order http://en.cppreference.com/w/cpp/language/eval_order). In general, you cannot be sure of the order of evaluation for a statement of form say, f() == g(). The relative order in which f & g are called is unspecified. Also, the behavior can easily become undefined if you violate certain conditions in f & g (as in the case above).
- 12y ago

twotwotwo 12y ago

These are especially interesting to me for the rules--it is not crazy hard to add things to Go's `go vet` tool (or write other checks outside vet with `go/ast`, etc.), and I wonder if "assigned variable twice consecutively" or "foo() == foo() looks suspicious" are good candidates.