PVS-Studio 与 Chromium

PVS-Studio VS Chromium

谷歌这一次取得了胜利。更准确的说,Chromium 项目的源代码取得了胜利。Chromium 是我们用 PVS-Studio 检查过的最出色的项目之一。

Chromium是谷歌公司开发的一款开源 web 浏览器,旨在为用户提供快速和安全的互联网访问。Chromium 是谷歌 Chrome 浏览器的基础。另外,Chromium 还是谷歌 Chrome 以及某些其它备用 web 浏览器的初步版本。

从编程的角度来看,Chromium 是一款包含 473 个项目的解决方案。C/C++ 源代码的一般体积为 460 MB,行数量难以统计。

这 460 MB 的容量中包括大量不同的库。如果将这些库排除在外,其大小约为 155 MB。容量减少许多,但仍具有许多行。要知道,什么事情都是相对的。其中许多库是由 Chromium 开发人员在创建 Chromium 本身时创建的。尽管这些库独立存在,但我们仍会将其与浏览器联系起来。

在测试PVS-Studio期间,Chromium 是我研究过的质量最高、规模最大的项目。在处理 Chromium 项目时,我们其实并不确定要检查什么内容:我们在 PVS-Studio 中发现并解决了与 C++ 文件分析和特定项目结构支持相关的多个错误。

Chromium 中使用的许多方法显示了其源代码的质量。例如,大多数编程人员使用下面的结构来确定阵列中项目的数量:

int XX[] = { 1, 2, 3, 4 };
size_t N = sizeof(XX) / sizeof(XX[0]);

通常,可通过下面的宏来实现:

#define count_of(arg) (sizeof(arg) / sizeof(arg[0]))

这是一个十分高效和实用的宏。老实说,我自己一直在使用该宏。但是,它可能会导致发生错误,这是因为你可能会意外地将一个简单的指针发送到该宏,而且它并未注意。让我用下面的示例来解释一下这一过程:

void Test(int C[3])
{
  int A[3];
  int *B = Foo();
  size_t x = count_of(A); // Ok
  x = count_of(B); // Error
  x = count_of(C); // Error
}

count_of(A) 结构正常工作,并返回了阵列 A 中的项目数量(此处为 3)。

但是,如果你意外地向指针应用 count_of(),那么结果将是一个没有意义的值。问题是,如果出现奇怪的 count_of(B) 结构,宏不会向编程人员发出警告。该情况看上去很不可靠和牵强,我已经在多种应用中碰到过此问题。比如,考虑 Miranda IM 项目中的下列代码:

#define SIZEOF(X) (sizeof(X)/sizeof(X[0]))
int Cache_GetLineText(..., LPTSTR text, int text_size, ...)
{
  ...
  tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, SIZEOF(text), 0);
  ...
}

这些代码将存在于你的代码中,你最好想办法加以应对。当试图计算作为参数传送的阵列的大小时,更容易发生错误:

void Test(int C[3])
{
  x = count_of(C); // Error
}
根据 C++ 标准,'C' 变量是一个简单的指针,而不是阵列。因此,你可能在程序中看到只有一部分阵列得到了处理。 

既然已经谈到了此类错误,我来告诉你一个能够确定传输阵列大小的方法。你应该根据参考传输阵列:

void Test(int (&C)[3])
{
  x = count_of(C); // Ok
}

现在,count_of(C) 表达式的值是 3。

让我们返回至 Chromium。它使用一个宏来避免上面描述的错误。实施方式如下:

template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define arraysize(array) (sizeof(ArraySizeHelper(array)))

具体的工作原理如下:模板函数 ArraySizeHelper 接收随机类型的阵列(长度为 N)。函数返回长度为 N 且包含 'char' 项目的阵列的参考。目前还没有实施该函数,因为我们不需要它。对于 sizeof() 操作符来说,已经足够定义 ArraySizeHelper 函数。'arraysize' 宏计算 ArraySizeHelper 函数返回的阵列字节大小。该大小为我们要计算长度的阵列中的项目数量。

如果你为此感到抓狂,请相信我 - 这真得有用。而且,它的效果比我们上面介绍的 'count_of()' 宏更好。由于 ArraySizeHelper 函数根据参考来选择阵列,你不能向其传送简单的指针。我们来写一个测试代码:

template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define arraysize(array) (sizeof(ArraySizeHelper(array)))

void Test(int C[3])
{
  int A[3];
  int *B = Foo();
  size_t x = arraysize(A); // Ok
  x = arraysize(B); // Compilation error
  x = arraysize(C); // Compilation error
}

系统不会对该错误的代码进行编译。我认为,能够在编译阶段防止可能发生的错误是一件很酷的事情。这是一个很不错的示例,反映了该编程方法的质量。在这里我要向谷歌的开发人员致敬。

我再提供一个不同类型的示例,它同样展示了代码的质量。

if (!file_util::Delete(db_name, false) &&
    !file_util::Delete(db_name, false)) {
  // Try to delete twice. If we can't, fail.
  LOG(ERROR) << "unable to delete old TopSites file";
  return false;
}

许多编程人员可能会认为该代码很奇怪。尝试两次删除一个文件有什么意义?当然有意义。编写人员得到了启示,并理解了软件存在的本质。肯定能够删除一个文件或者根本不能删除文件,这只存在于教科书或某些抽象世界里。在真实系统中经常发生这种情况,即一个文件暂时不能删除,而稍后却能够删除。这可能涉及许多原因:杀毒软件、病毒、版本控制系统等。编程人员一般不会考虑此类情况。他们认为,如果你不能删除一个文件,则说明该文件永远不能删除。但是,如果你希望确保一切顺利,并避免目录混乱,你应当考虑这些外来因素。我也遇到过相同的情况,即在 1000 此运行中无法删除一个文件。解决方法是相同的 – 为了以防万一,我在中间添加了 Sleep(0)。

PVS-Studio 执行的检查怎么样?Chromium 代码可能是我见过的质量最高的代码。测试期间发生的错误率很低,由此可以证明这一点。如果考虑一般情况下的数量,当然还是非常多的。但是,如果用错误数量除以代码数量,则几乎没有错误。这些错误都是什么呢?这些都是最普通的错误。下面给出了几个具体的示例:

V512 A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. platform time_win.cc 116
void NaCl::Time::Explode(bool is_local, Exploded* exploded) const {
  ...
  ZeroMemory(exploded, sizeof(exploded));
  ...
}

每个人都会发生打印错误。在本例中,一个星号丢失。它必须是 sizeof(*exploded)。

V502  Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator.  views  custom_frame_view.cc  400
static const int kClientEdgeThickness;
int height() const;
bool ShouldShowClientEdge() const;

void CustomFrameView::PaintMaximizedFrameBorder(gfx::Canvas* canvas) {
  ...
  int edge_height = titlebar_bottom->height() -
                    ShouldShowClientEdge() ? kClientEdgeThickness : 0;
  ...
}

The insidious operator "?:" has a lower priority than subtraction. There must be additional parentheses here:

int edge_height = titlebar_bottom->height() -
                  (ShouldShowClientEdge() ? kClientEdgeThickness : 0);

A meaningless check.

V547  Expression 'count < 0' is always false. Unsigned type value is never < 0.  ncdecode_tablegen  ncdecode_tablegen.c  197
static void CharAdvance(char** buffer, size_t* buffer_size, size_t count) {
  if (count < 0) {
    NaClFatal("Unable to advance buffer by count!");
  } else {
  ...
}

"count < 0" 条件一直是错误的。该保护不起作用,部分缓冲可能溢出。另外,该示例还展示了如何使用静态分析工具来搜索漏洞。入侵者能够快速发现包含错误的代码片段,以便进行进一步的调查。下面是另外一个与安全问题相关的代码示例:

V511  The sizeof() operator returns size of the pointer, and not of the array, in 'sizeof (salt)' expression.  common  visitedlink_common.cc  84
void MD5Update(MD5Context* context, const void* buf, size_t len);

VisitedLinkCommon::Fingerprint VisitedLinkCommon::ComputeURLFingerprint(
  ...
 const uint8 salt[LINK_SALT_LENGTH])
{
  ...
  MD5Update(&ctx, salt, sizeof(salt));
  ...
}

指针占用多少字节数,MD5Update() 函数便处理多少字节数。这是数据加密系统中的一个潜在漏洞,不是吗?我不知道它是否会造成什么危险;但是,从入侵者的角度来说,这不利于完整分析。

正确的代码应为:

MD5Update(&ctx, salt, sizeof(salt[0]) * LINK_SALT_LENGTH);

或者:

VisitedLinkCommon::Fingerprint VisitedLinkCommon::ComputeURLFingerprint(
  ...
 const uint8 (&salt)[LINK_SALT_LENGTH])
{
  ...
  MD5Update(&ctx, salt, sizeof(salt));
  ...
}

另外一个关于错误打印的示例:

V501  There are identical sub-expressions 'host != buzz::XmlConstants::str_empty ()' to the left and to the right of the '&&' operator.  chromoting_jingle_glue  iq_request.cc  248
void JingleInfoRequest::OnResponse(const buzz::XmlElement* stanza) {
  ...
  std::string host = server->Attr(buzz::QN_JINGLE_INFO_HOST);
  std::string port_str = server->Attr(buzz::QN_JINGLE_INFO_UDP);
  if (host != buzz::STR_EMPTY && host != buzz::STR_EMPTY) {
  ...
}

The port_str variable must be actually checked as well:

if (host != buzz::STR_EMPTY && port_str != buzz::STR_EMPTY) {

A bit of classics:

V530  The return value of function 'empty' is required to be utilized.  chrome_frame_npapi  np_proxy_service.cc  293
bool NpProxyService::GetProxyValueJSONString(std::string* output) {
  DCHECK(output);
  output->empty();
  ...
}

It must be: output->clear();

And here is even the handling of a null pointer:

V522  Dereferencing of the null pointer 'plugin_instance' might take place. Check the logical condition.  chrome_frame_npapi  chrome_frame_npapi.cc  517
bool ChromeFrameNPAPI::Invoke(...)
{
  ChromeFrameNPAPI* plugin_instance =
    ChromeFrameInstanceFromNPObject(header);
  if (!plugin_instance && (plugin_instance->automation_client_.get()))
    return false;
  ...  
}

另外一个不起作用的检查示例:

V547  Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0.  browser  idle_win.cc  23
IdleState CalculateIdleState(unsigned int idle_threshold) {
  ...
  DWORD current_idle_time = 0;
  ...
  // Will go -ve if we have been idle for a long time (2gb seconds).
  if (current_idle_time < 0)
    current_idle_time = INT_MAX;
  ...
}

好的,我们今天到此为止。我可以继续讲解,但这开始变得无聊了。请记住,所有这些只是关于 Chromium 本身。但是,也存在具有类似于下面错误的测试:

V554  Incorrect use of auto_ptr. The memory allocated with 'new []' will be cleaned using 'delete'.  interactive_ui_tests  accessibility_win_browsertest.cc  306
void AccessibleChecker::CheckAccessibleChildren(IAccessible* parent) {
  ...
  auto_ptr<VARIANT> child_array(new VARIANT[child_count]);
  ...
}

Chromium 平台有大量的库,库的总容量要远远大于 Chromium 本身。同时还存在许多有趣的片段。很明显,包含错误的代码可能不能随意使用,但它们仍然是错误。考虑其中的一个示例(ICU 库):

V547 Expression '* string != 0 || * string != '_'' is always true. Probably the '&&' operator should be used here.  icui18n ucol_sit.cpp 242
U_CDECL_BEGIN static const char* U_CALLCONV
_processVariableTop(...)
{
  ...
  if(i == locElementCapacity && (*string != 0 || *string != '_')) {
    *status = U_BUFFER_OVERFLOW_ERROR;
  }
  ...
}

The "(*string != 0 || *string != '_')" expression is always true. Perhaps it must be: (*string == 0 || *string == '_').

结论

PVS-Studio 被击败了。Chromium 的源代码是我们分析过的最好的代码之一。我们在 Chromium 中几乎没有发现什么。更确切地说,我们发现了许多错误,本文只列举了其中一部分。但是我们要知道,如果将这些错误分布到 460 MB 的源代码中,则几乎没有什么错误。

附言

我来回答一下这个问题:就发现的错误,我们会通知 Chromium 开发人员吗?不会。这是一项较大规模的工作,我们不能免费做这样一项工作。检查 Chromium 与检查 Miranda IM检查 Ultimate Toolbox 有着很大的不同。这是一项艰苦的工作,我们必须研究所有讯息,并决定每个具体的案例中是否存在错误。要做到这一点,我们必须十分了解该项目。Chromium 开发人员可以阅读本文,如果他们有兴趣,则可以自己分析该项目,并对所有诊断信息进行研究。是的,要执行该操作,他们要购买 PVS-Studio。对于所有谷歌部门来说,这都算不了什么。

有关编译器优化的更完整信息,请参阅优化通知
类别: