About half a year ago we checked the Chromium project and wrote an article about it. The PVS-Studio analyzer naturally keeps developing, and in the new Chromium version we have found some errors undetected before. Many errors, of course, refer not to the Chromium project itself but to libraries it employs. But in this article I want to show you how the analyzer's capabilities have improved and not tell you about what we have found in this or that part of Chromium. That's why I will give messages together.
So, in this post we will enumerate the new defects found by the analyzer. This is far from all of the things found, we've only glanced through the report and included only what we found at first glance. If you want a more thorough analysis of Chromium or its libraries, we can provide developers with the full version of PVS-Studio for a time so that they perform it themselves. By the way, go by this link and perhaps you will get interested in the opportunity to try the full PVS-Studio version too: http://www.viva64.com/en/b/0092/
Fragment N1
std::string TestFileIO::TestParallelReads() {
...
const char* expected_result_1 =
"__border__abc__border__";
const char* expected_result_2 =
"__border__defghijkl__border__";
if (strncmp(extended_buf_1, expected_result_1,
sizeof(expected_result_1)) != 0 ||
strncmp(extended_buf_2, expected_result_2,
sizeof(expected_result_2)) != 0) {
...
}PVS-Studio's diagnostic messages:
V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. ppapi_tests test_file_io.cc 759
V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. ppapi_tests test_file_io.cc 761
Calls of the strncmp() function in this code compare only the first characters and not the whole strings. To calcluate strings' lengths the programmer tried to use the sizeof() operator absolutely inappropriate for this purpose. The sizeof() operator will calculate the pointer's size instead of the number of bytes in a string.
Fragment N2
int AffixMgr::parse_convtable(..., const char * keyword)
{
...
if (strncmp(piece, keyword, sizeof(keyword)) != 0) {
...
}PVS-Studio's diagnostic message:
V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. hunspell affixmgr.cxx 3545
Again, the mistake here is that only the first 4 or 8 bytes will be compared depending on the pointer's size.
Fragment N3
#define SEC_ASN1_CHOICE 0x100000
typedef struct sec_ASN1Template_struct {
unsigned long kind;
...
} SEC_ASN1Template;
PRBool SEC_ASN1IsTemplateSimple(
const SEC_ASN1Template *theTemplate)
{
...
if (!theTemplate->kind & SEC_ASN1_CHOICE) {
...
}PVS-Studio's diagnostic message:
V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. nss secasn1u.c 121
The error is caused by the issue of operation priorities. The correct code is this one:
if (!(theTemplate->kind & SEC_ASN1_CHOICE)) {Fragment N4
bool GetPlatformFileInfo(...) {
...
info->is_directory =
file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0;
...
}PVS-Studio's diagnostic message:
V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 219
The error is caused by the issue of operation priorities. The correct code is this one:
info->is_directory =
(file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0;Fragment N5
WebRtc_Word32
interpolator::SupportedVideoType(VideoType srcVideoType,
VideoType dstVideoType)
{
...
if ((srcVideoType != kI420) ||
(srcVideoType != kIYUV) ||
(srcVideoType != kYV12))
{
return -1;
}
...
}PVS-Studio's diagnostic message:
V547 Expression is always true. Probably the '&&' operator should be used here. webrtc_vplib interpolator.cc 119
The (A != 123 || A!= 321) - like condition is always true. There is obviously a misprint here and the condition must look in a different way.
Fragment N6
static GLenum
get_temp_image_type(GLcontext *ctx, GLenum baseFormat)
{
...
if (ctx->DrawBuffer->Visual.redBits <= 8)
return GL_UNSIGNED_BYTE;
else if (ctx->DrawBuffer->Visual.redBits <= 8)
return GL_UNSIGNED_SHORT;
else
return GL_FLOAT;
...
}PVS-Studio's diagnostic message:
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2520, 2522. osmesa meta.c 2520
One and the same check is performed twice. Perhaps it should look this way:
if (ctx->DrawBuffer->Visual.redBits <= 8)
return GL_UNSIGNED_BYTE;
else if (ctx->DrawBuffer->Visual.redBits <= 16)
return GL_UNSIGNED_SHORT;Fragment N7
WebRtc_Word32 ModuleFileUtility::UpdateWavHeader(OutStream& wav)
{
...
if(STR_CASE_CMP(codec_info_.plname, "L16") == 0)
{
res = WriteWavHeader(wav, codec_info_.plfreq, 2,
channels, kWaveFormatPcm, _bytesWritten);
} else if(STR_CASE_CMP(codec_info_.plname, "PCMU") == 0) {
res = WriteWavHeader(wav, 8000, 1, channels,
kWaveFormatMuLaw, _bytesWritten);
} else if(STR_CASE_CMP(codec_info_.plname, "PCMU") == 0) {
res = WriteWavHeader(wav, 8000, 1, channels,
kWaveFormatALaw, _bytesWritten);
} else {
...
}PVS-Studio's diagnostic message:
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1324, 1327. media_file media_file_utility.cc 1324
The 'plname' variable is compared to the "PCMU" string twice in a row. Most likely, another string must be used for the second time.
Fragment N8
enum ContentSettingsType;
struct EntryMapKey {
ContentSettingsType content_type;
...
};
bool OriginIdentifierValueMap::EntryMapKey::operator<(
const OriginIdentifierValueMap::EntryMapKey& other) const {
if (content_type < other.content_type)
return true;
else if (other.content_type > content_type)
return false;
return (resource_identifier < other.resource_identifier);
}PVS-Studio's diagnostic message:
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 61, 63. browser content_settings_origin_identifier_value_map.cc 61
The condition looks this way for the first time: "A < B"; and this way for the second time: "B > A". Thus, the check is meaningles. There is obviously a misrpint in the code.
Fragment N9
WebRtc_Word32
RTPReceiverVideo::ReceiveH263Codec(...)
{
...
if (IP_PACKET_SIZE < parsedPacket.info.H263.dataLength +
parsedPacket.info.H263.insert2byteStartCode? 2:0)
...
}PVS-Studio's diagnostic message:
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. rtp_rtcp rtp_receiver_video.cc 480
The '?:' operator has a lower priority than the '+' operator. As a result, the condition works in a way other than the programmer expected. The correct condition must look this way:
if (IP_PACKET_SIZE < parsedPacket.info.H263.dataLength +
(parsedPacket.info.H263.insert2byteStartCode ? 2:0))The same error is here:
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. rtp_rtcp rtp_receiver_video.cc 504
Fragment N10
static int
xmlXPathCompOpEvalFirst(...)
{
...
total += xmlXPathCompOpEvalFirst(...);
...
total =+ xmlXPathCompOpEvalFilterFirst(ctxt, op, first);
...
}PVS-Studio's diagnostic message:
V588 The expression of the 'A =+ B' kind is utilized. Consider reviewing it, as it is possible that 'A += B' was meant. libxml xpath.c 12676
As you may see from the first line, a certain sum is being calculated in the total variable. But then there is a misprint and we have "=+" instead of "+=".
Fragment N11
static VisiblePosition updateAXLineStartForVisiblePosition(...)
{
...
tempPosition = startPosition.previous();
if (tempPosition.isNull() || tempPosition.isNull())
break;
...
}PVS-Studio's diagnostic message:
V501 There are identical sub-expressions 'tempPosition.isNull ()' to the left and to the right of the '||' operator. webcore_remaining accessibilityobject.cpp 489
Strange code.
Fragment N12
TEST(SharedMemoryTest, MultipleThreads) {
...
int threadcounts[] = { 1, kNumThreads };
for (size_t i = 0;
i < sizeof(threadcounts) / sizeof(threadcounts); i++) {
...
}PVS-Studio's diagnostic message:
V501 There are identical sub-expressions 'sizeof (threadcounts)' to the left and to the right of the '/' operator. base_unittests shared_memory_unittest.cc 231
Because of the error, the loop in the test function performs only one iteration. This is the correct loop:
for (size_t i = 0;
i < sizeof(threadcounts) / sizeof(*threadcounts); i++) {Fragment N13
bool
ir_algebraic_visitor::reassociate_constant(...)
{
if (ir1->operands[0]->type->is_matrix() ||
ir1->operands[0]->type->is_matrix() ||
ir2->operands[1]->type->is_matrix() ||
ir2->operands[1]->type->is_matrix())
return false;
}PVS-Studio's diagnostic message:
V501 There are identical sub-expressions 'ir1->operands [0]->type->is_matrix ()' to the left and to the right of the '||' operator. mesa ir_algebraic.cpp 189
The code seems to have been written through the Copy-Paste method, while the programmer fixed the indexes incorrectly. This is how the comparison must look:
if (ir1->operands[0]->type->is_matrix() ||
ir1->operands[1]->type->is_matrix() ||
ir2->operands[0]->type->is_matrix() ||
ir2->operands[1]->type->is_matrix())Fragment N15
#define FRAMESAMPLES_HALF 240
#define FRAMESAMPLES 480
typedef struct {
...
WebRtc_Word16 realFFT[FRAMESAMPLES_HALF];
WebRtc_Word16 imagFFT[FRAMESAMPLES_HALF];
} ISACUBSaveEncDataStruct;
int WebRtcIsac_EncodeStoredDataUb12(...)
{
...
for(n = 0; n < FRAMESAMPLES; n++)
{
realFFT[n] = (WebRtc_Word16)
(scale * (float)ISACSavedEnc_obj->realFFT[n] + 0.5f);
imagFFT[n] = (WebRtc_Word16)
(scale * (float)ISACSavedEnc_obj->imagFFT[n] + 0.5f);
}
...
}PVS-Studio's diagnostic messages:
V557 Array overrun is possible. The value of 'n' index could reach 479. iSAC encode.c 1307
V557 Array overrun is possible. The value of 'n' index could reach 479. iSAC encode.c 1308
Array overruns occur in the loop. The loop must search through FRAMESAMPLES_HALF items only.
Fragment N16
static int
coff_helper_gasflags(...)
{
...
case 'd':
datasect = 1;
load = 1;
readonly = 0;
case 'x':
code = 1;
load = 1;
break;
...
}PVS-Studio's diagnostic message:
V519 The 'load' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1346, 1350. yasm coff-objfmt.c 1350
The 'break;' operator seems to be missing here.