To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Python and Ruby implementations compare…

Python and Ruby implementations compared by the error density

July 22, 2016
Author:

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.

0414_RubyCPython/image1.png

Introduction

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:501

And 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".

Python

0414_RubyCPython/image2.png

Fragment N1

#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.

  • V547 Expression 's->sock_fd < 0' is always false. Unsigned type value is never < 0. _ssl.c 1702
  • V547 Expression 'sock->sock_fd < 0' is always false. Unsigned type value is never < 0. _ssl.c 2018

Fragment N2

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.

Fragment N3

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.

Fragment N4

#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:

  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!type' and 'type'. digest.c 167
  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!cipher' and 'cipher'. evp_enc.c 120

Fragment N5

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.

Fragment N6

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.

Ruby

0414_RubyCPython/image3.png

Fragment N1

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.

Fragment N2

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.

Fragment N3

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.

Fragment N4

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.

Overall results

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:

0414_RubyCPython/image4.png

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:

0414_RubyCPython/image5.png

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.

Conclusion

0414_RubyCPython/image6.png

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.

Popular related articles
Appreciate Static Code Analysis!

Date: 10.16.2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
PVS-Studio for Java

Date: 01.17.2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
PVS-Studio ROI

Date: 01.30.2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
The Last Line Effect

Date: 05.31.2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: 10.22.2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: 07.31.2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
Static analysis as part of the development process in Unreal Engine

Date: 06.27.2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: 11.21.2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
Free PVS-Studio for those who develops open source projects

Date: 12.22.2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
The way static analyzers fight against false positives, and why they do it

Date: 03.20.2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept