Which programming language to start learning? Python or Ruby? Which one is better? Django or Ruby on Rails? Such questions can often be found on IT forums around the world. I suggest comparing not the languages themselves, but their reference implementations: CPython and MRI. In this article, we are going to cover the errors that were found by PVS-Studio in these projects.
We took the latest versions of the source code from the repositories (Ruby, Python) for the analysis. The project was scanned with PVS-Studio v6.06 static code analyzer. Python can be easily compiled in Visual Studio; for Ruby you can use a Standalone version in the compilation monitoring mode.
There weren't very many glaring errors: the majority of warnings are connected with the usage of macros, which get expanded into quite a suspicious code, from the point of view of the analyzer, but rather innocent from a developer's point of view. We could start a long discussion on whether macros bring harm or good, but we can say for sure that the analyzer doesn't like them much. To get rid of some annoying macro, there is an option to suppress false positives. It's enough to write:
//-V:RB_TYPE_P:501And all the warnings by V501 diagnostic, where there is RB_TYPE_P macro will disappear.
Update. This article contains a few inaccuracies. Please see the updated version here: "Update on Analysis Results for CPython and Ruby".
#ifdef MS_WINDOWS
typedef SOCKET SOCKET_T;
#else
typedef int SOCKET_T;
#endif
typedef struct {
PyObject_HEAD
SOCKET_T sock_fd; /* Socket file descriptor */
....
} PySocketSockObject;
static int
internal_select(PySocketSockObject *s,
int writing,
_PyTime_t interval,
int connect)
{
....
if (s->sock_fd < 0) // <=
return 0;
....
}V547 Expression 's->sock_fd < 0' is always false. Unsigned type value is never < 0. socketmodule.c 655
The SOCKET type in Windows is unsigned, so comparing it against null is meaningless. The check of the socket() function returned a correct descriptor, it's necessary to compare its value with INVALID_SOCKET. It is worth noting that this comparison would work correctly in Linux, because there as the socket type, we have a signed type int used, and the value -1 indicates an error. Nevertheless, it's better to use special macros or constants to check.
Several more similar checks that the analyzer issued warnings for.
int ASN1_PRINTABLE_type(const unsigned char *s, int len)
{
int c;
int ia5 = 0;
....
if (!(((c >= 'a') && (c <= 'z')) ||
((c >= 'A') && (c <= 'Z')) ||
(c == ' ') || // <=
((c >= '0') && (c <= '9')) ||
(c == ' ') || (c == '\'') || // <=
(c == '(') || (c == ')') ||
(c == '+') || (c == ',') ||
(c == '-') || (c == '.') ||
(c == '/') || (c == ':') ||
(c == '=') || (c == '?')))
ia5 = 1;
....
}V501 There are identical sub-expressions '(c == ' ')' to the left and to the right of the '||' operator. a_print.c 77
A typical example of an error that occurred as a result of Copy-Paste. Quite often, when using large amounts of copied blocks, a programmer's attention falters and they forget to change a variable or a constant in them. For example, in this case in a large conditional expression, the programmer confused the values that the variable c is compared with. We cannot say for sure, but it seems that the symbol of double quotation '"' was forgotten.
static PyObject *
semlock_acquire(SemLockObject *self, PyObject *args, PyObject *kwds)
{
....
HANDLE handles[2], sigint_event;
....
/* prepare list of handles */
nhandles = 0;
handles[nhandles++] = self->handle;
if (_PyOS_IsMainThread()) {
sigint_event = _PyOS_SigintEvent();
assert(sigint_event != NULL);
handles[nhandles++] = sigint_event;
}
/* do the wait */
Py_BEGIN_ALLOW_THREADS
if (sigint_event != NULL) // <=
ResetEvent(sigint_event);
....
}V614 Potentially uninitialized pointer 'sigint_event' used. semaphore.c 120
In case the function _PyOS_IsMainThread() returns false, the pointer sigint_event will remain uninitialized. This will result in undefined behavior. Such an error can easily be overlooked in the debug version, where a pointer is most likely to be initialized by a null.
#define BN_MASK2 (0xffffffffffffffffLL)
int BN_mask_bits(BIGNUM *a, int n)
{
....
a->d[w] &= ~(BN_MASK2 << b); // <=
....
}V610 Undefined behavior. Check the shift operator '<<'. The left operand '(0xffffffffffffffffLL)' is negative. bn_lib.c 796
Despite the fact that the code works in most cases, this expression is considered to be undefined behavior according to the standard. You can find more details about the shifts of negative numbers in the article written by Andrey Karpov "Wade not in unknown waters. Part three". It's up to you to decide where it's necessary to avoid constructions whose results aren't guaranteed by the standard; but it's better not to do it at all; the analyzer agrees on that too.
static PyObject *
binascii_b2a_qp_impl(PyModuleDef *module,
Py_buffer *data,
int quotetabs,
int istext,
int header)
{
Py_ssize_t in, out;
const unsigned char *databuf;
....
if ((databuf[in] > 126) ||
(databuf[in] == '=') ||
(header && databuf[in] == '_') ||
((databuf[in] == '.') && (linelen == 0) &&
(databuf[in+1] == '\n' || databuf[in+1] == '\r' ||
databuf[in+1] == 0)) ||
(!istext && ((databuf[in] == '\r') ||
(databuf[in] == '\n'))) ||
((databuf[in] == '\t' || databuf[in] == ' ') &&
(in + 1 == datalen)) ||
((databuf[in] < 33) &&
(databuf[in] != '\r') && (databuf[in] != '\n') &&
(quotetabs ||
(!quotetabs && ((databuf[in] != '\t') && // <=
(databuf[in] != ' '))))))
{
....
}
....
}V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'quotetabs' and '!quotetabs'. binascii.c 1453
This fragment is not erroneous, nevertheless, we should take a closer look at it. The warning is mostly a recommendation: the expression 'A || (!A && B)' can be simplified to 'A || B': , which will make this quite complicated code easier to read.
Similar warnings:
static int dh_cms_set_peerkey(....)
{
....
int atype;
....
/* Only absent parameters allowed in RFC XXXX */
if (atype != V_ASN1_UNDEF && atype == V_ASN1_NULL)
goto err;
....
}V590 Consider inspecting the 'atype != - 1 && atype == 5' expression. The expression is excessive or contains a misprint. dh_ameth.c 670
It shouldn't seem odd, that errors in logical expressions occur even in large projects. A logical expression is excessive here, and it can be simplified to 'atype == V_ASN1_NULL'. Judging by the context, there is no error here, but such code looks really suspicious.
static void cms_env_set_version(CMS_EnvelopedData *env)
{
....
if (env->originatorInfo || env->unprotectedAttrs)
env->version = 2;
env->version = 0;
}V519 The 'env->version' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 907, 908. cms_env.c 908
It's hard to say what the author meant writing this code. Perhaps else is omitted here. At this point there is no sense in if, as the value of the 'env->version' variable will be rewritten in any case.
int
_PyState_AddModule(PyObject* module, struct PyModuleDef* def)
{
PyInterpreterState *state;
if (def->m_slots) {
PyErr_SetString(PyExc_SystemError,
"PyState_AddModule called on module with slots");
return -1;
}
state = GET_INTERP_STATE();
if (!def)
return -1;
....
}V595 The 'self->extra' pointer was utilized before it was verified against nullptr. Check lines: 917, 923. _elementtree.c 917
This is a traditional error, related to the null pointer dereferencing, which we find almost in every project. Firstly, in the expression 'def->m_slots' the programmer accessed by some address, and then it turned out that this address might have been null. As a result the verification against nullptr won't work, as we'll have the null pointer dereference, which will lead to undefined program behavior and to its crash, for instance.
static void
vm_set_main_stack(rb_thread_t *th, const rb_iseq_t *iseq)
{
VALUE toplevel_binding = rb_const_get(rb_cObject,
rb_intern("TOPLEVEL_BINDING"));
rb_binding_t *bind;
rb_env_t *env;
GetBindingPtr(toplevel_binding, bind);
GetEnvPtr(bind->env, env);
vm_set_eval_stack(th, iseq, 0, &env->block);
/* save binding */
if (bind && iseq->body->local_size > 0) {
bind->env = vm_make_env_object(th, th->cfp);
}
}V595 The 'bind' pointer was utilized before it was verified against nullptr. Check lines: 377, 382. vm.c 377
A similar error was also encountered in the Ruby project. The check 'if (bind)' won't be of much help, because bind was dereferenced a little earlier in the code. There were more than 30 warnings of this kind, so there is no point in listing them all here.
static int
code_page_i(....)
{
table = realloc(table, count * sizeof(*table));
if (!table) return ST_CONTINUE;
....
}V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'table' is lost. Consider assigning realloc() to a temporary pointer. file.c 169
In this fragment we see that the value of realloc is saved in the same variable, which is used as an argument. In case realloc returns nullptr, the initial pointer value will be lost, which will lead to a memory leak.
static int
w32_symlink(UINT cp, const char *src, const char *link)
{
....
BOOLEAN ret;
typedef DWORD (WINAPI *create_symbolic_link_func)
(WCHAR*, WCHAR*, DWORD);
static create_symbolic_link_func create_symbolic_link =
(create_symbolic_link_func)-1;
....
ret = create_symbolic_link(wlink, wsrc, flag);
ALLOCV_END(buf);
if (!ret) {
int e = GetLastError();
errno = map_errno(e);
return -1;
}
return 0;
}V724 Converting type 'DWORD' to type 'BOOLEAN' can lead to a loss of high-order bits. Non-zero value can become 'FALSE'. win32.c 4974
The BOOLEAN type is used in the WinAPI as a logical type. It is declared in the following way:
typedef unsigned char BYTE;
typedef BYTE BOOLEAN;DWORD is a 32-bit unsigned number. That's why if we cast DWORD 0xffffff00 value to BOOLEAN (or any other, whose lowest bit is zero), then it will become 0, i.e. FALSE.
static VALUE
rb_str_split_m(int argc, VALUE *argv, VALUE str)
{
....
char *ptr = RSTRING_PTR(str);
long len = RSTRING_LEN(str);
long start = beg;
....
if (ptr+start == ptr+len)
start++;
....
}V584 The 'ptr' value is present on both sides of the '==' operator. The expression is incorrect or it can be simplified. string.c 7211
In both parts of the comparison we have ptr addition, therefore it can be removed:
if (start == len)But most likely, there is no error in this fragment. However, we see quite often that programmers compare two different variables in such expressions. That's why such comparisons are always worth reviewing.
Having analyzed all the warnings of the general analysis diagnostics, and removed all the false positives, I have come to the following conclusion concerning the error density:
The majority of warnings in Ruby were issued by the V610 diagnostic (369 warnings!), but even if we exclude them, the situation won't change much: Python takes the lead over Ruby in the number of suspicious fragments.
The most frequent diagnostic turned out to be V595 - there were 17 warnings in Python, and 37 in Ruby.
Of course, it's much more interesting to look at the error density ratio. Python also leaves Ruby well behind in this category. Here are the results of the evaluations presented as a table:
It may seem that the number of errors is quite large. But it is not so. Firstly, not all of the bugs are critical. For example, the V610 diagnostic that we have already mentioned, detects errors from the point of view of the C++ language. However, in practice for the set of compilers the result can always be correct. Although these errors are still bugs, they don't affect the program in any way. Secondly, we should take into account the size of the code. That's why we can say that the quality of these projects is rather high. At this point this may be rather subjective, because previously we haven't evaluated the error density of these projects. We'll try to do that in the future, so that we can later compare the result of the checks.
Python and Ruby are extremely popular: millions of developers use them to write code. It's hard to find a large number of errors in a project when it is so actively used, regularly tested by another static analysis tool (both projects get checked by Coverity) and has community support. Nonetheless, PVS-Studio managed to find several suspicious fragments. We should understand that these are regular checks which can make the life of programmers much easier. The ideal, is to fix the error before the edits get to repository and release - and a static analyzer can help best of all here.
I suggest running PVS-Studio on your projects.