mardi 22 mars 2016

Analyzing Samba with PVS-Studio on Linux

If you have followed the last developement in C/C++ static analysis tools you must have heard of PVS-Studio. I heard of them through the articles they publish on their site where they analyze open source projects. They have analyzed quite big projects including the Linux kernel, Qt, Unreal, … and they have always managed to find crazy bugs that have been siting there for some time, undetected. Typos, bad copy-paste, undefined behaviours, non-sense code, syntax errors that miraculously still compile… As John Carmack said:

Everything that is syntactically legal that the compiler will accept will eventually wind up in your codebase.

Unfortunately, the tool is advertized as Windows-only. The program comes in the form of a Visual Studio plugin or a separate independent program if you don't have the former. I first used it back in 2014 on a relatively large C++ codebase used internally in the computer graphics department of my university in Lyon (LIRIS). We were using Visual Studio (which I normally rarely use) so I thought I should give it a try. I was really pleased with the results and kept checking the PVS-Studio website for more articles.

Two years and several PVS-Studio articles later I started working on Samba. The whole project is about 2 millions lines of C code and I thought it would be a good candidate for PVS-Studio. A static analysis tool shouldn't have too much platform-specific code so I started thinking about it. The analyzer works on preprocessed code so it needs to run the preprocessor on your sources and for that it needs all your preprocessor flags, macros and includes path. Gathering this automatically can be painful. For this step I wrote a strace-based script that "spies" your build tool for compiler calls, that way it should be build-tool agnostic. You can find the latest version of this tool on github.

I sent the script to the PVS-Studio guys and after some back and forth, I was given an experimental Linux build of PVS-Studio (thanks again!). The script now covers all the analyzing process from gathering compiler flags, to analyzing, displaying and filtering the results.

Here's how you use it.

In order to not have to point to the license and binary at every use you can set up env variables.

    $ export PVS_LICENSE=~/prog/pvs/PVS-Studio.lic
    $ export PVS_BIN=~/prog/pvs/PVS-Studio

Go to your project directory and generate a config file for your C++11 project.

    $ pvs-tool genconf  -l C++11 pvs.cfg

If you need to configure the build before building, do it. Then trace the actual build (your build command should go after the --).

    $ pvs-tool trace    -- make -j8

This will output a "strace_out" file which have all the information we need. The analyze step will process that file to extract all compilation units and preprocessor flags, and run PVS-Studio on it.

    $ pvs-tool analyze  pvs.cfg
    pvs-tool: deleting existing log pvs.log...
    001/061 [ 0%] analyzing /hom../rtags/src/ClangIndexer.cpp...
    002/061 [ 1%] analyzing /hom../rtags/src/CompilerManager.cpp...
    003/061 [ 3%] analyzing /hom../rtags/src/CompletionThread.cpp...
    004/061 [ 4%] analyzing /hom../rtags/src/DependenciesJob.cpp...
    <...>
    061/061 [98%] analyzing /hom../rtags/src/rp.cpp...
    pvs-tool: analysis finished
    pvs-tool: cleaning output...
    pvs-tool: done (2M -> 0M)

The cleaning part removes duplicated lines and will drastically reduce the file size of big results.

You can now view the results, grouped by files

    $ pvs-tool view     pvs.log

The output is similar to gcc/make so it works as-is in e.g. the Emacs editor and I can use my usual builtin goto-error functions. You can disable diagnostics e.g.

    $ pvs-tool view -d V2006,V2008 pvs.log

By default it only shows level 1 errors but you can change it with -l.

You can look at the -h help messsage for more.


PVS-Studio found many problems in Samba. Most of them were false positives but this is expected when you use any static analysis tool on large codebase. The important thing is it also found real bugs. I'm going to share the most interesting ones along with their fix, in the form of diffs.

- if (memcmp(u0, _u0, sizeof(u0) != 0)) {
+ if (memcmp(u0, _u0, sizeof(*u0)) != 0) {
   printf("USER_MODALS_INFO_0 struct has changed!!!!\n");
   return -1;
  }

Here, the closing parenthesis was misplaced. The result of the sizeof comparaison was used as the compared memory size (always 1 byte). Also, we want the size of the type u0 points to, not the size of the pointer.


   handle_main_input(regedit, key);
   update_panels();
   doupdate();
- } while (key != 'q' || key == 'Q');
+ } while (key != 'q' && key != 'Q');

Here, we want to exit the loop on any case of the letter 'q'.


  uid = request->data.auth.uid;
 
- if (uid < 0) {
+ if (uid == (uid_t)-1) {
   DEBUG(1,("invalid uid: '%u'\n", (unsigned int)uid));
   return -1;
  }

Here we tested the uid_t type for negative values.

The sign of the uid_t type is left unspecified by POSIX. It's defined as an unsigned 32b int on Linux, therefore the < 0 check is always false.

For unsigned version of uid_t, in the comparaison uid == -1 the compiler will implicitely cast -1 to unsigned making it a valid test for both signed and unsigned version of uid_t. I've made the cast explicit because less magic is better in this case.


  DEBUG(4,("smb_pam_auth: PAM: Authenticate User: %s\n", user));
 
- pam_error = pam_authenticate(pamh, PAM_SILENT | allow_null_passwords ? 0 : PAM_DISALLOW_NULL_AUTHTOK);
+ pam_error = pam_authenticate(pamh, PAM_SILENT | (allow_null_passwords ? 0 : PAM_DISALLOW_NULL_AUTHTOK));
  switch( pam_error ){
   case PAM_AUTH_ERR:
    DEBUG(2, ("smb_pam_auth: PAM: Authentication Error for user %s\n", user));

Simple operator priority error.


  gensec_init();
  dump_args();
 
- if (check_arg_numeric("ibs") == 0 || check_arg_numeric("ibs") == 0) {
+ if (check_arg_numeric("ibs") == 0 || check_arg_numeric("obs") == 0) {
   fprintf(stderr, "%s: block sizes must be greater that zero\n",
     PROGNAME);
   exit(SYNTAX_EXIT_CODE);

Here the test was doing the same thing twice.


   if (!gss_oid_equal(&name1->gn_type, &name2->gn_type)) {
    *name_equal = 0;
   } else if (name1->gn_value.length != name2->gn_value.length ||
-      memcmp(name1->gn_value.value, name1->gn_value.value,
+      memcmp(name1->gn_value.value, name2->gn_value.value,
    name1->gn_value.length)) {
    *name_equal = 0;
   }

Here memcmp was called with the same pointer, thus comparing the same region of memory with itself.


  ioctl_arg.fd = src_fd;
  ioctl_arg.transid = 0;
  ioctl_arg.flags = (rw == false) ? BTRFS_SUBVOL_RDONLY : 0;
- memset(ioctl_arg.unused, 0, ARRAY_SIZE(ioctl_arg.unused));
+ memset(ioctl_arg.unused, 0, sizeof(ioctl_arg.unused));
  len = strlcpy(ioctl_arg.name, dest_subvolume,
         ARRAY_SIZE(ioctl_arg.name));
  if (len >= ARRAY_SIZE(ioctl_arg.name)) {

Here memset was given the size as a number of elements instead of a byte size.


  if (n + IDR_BITS < 31 &&
-     ((id & ~(~0 << MAX_ID_SHIFT)) >> (n + IDR_BITS))) {
+     ((id & ~(~0U << MAX_ID_SHIFT)) >> (n + IDR_BITS))) {
   return NULL;
  }

Using negative values on the left-side of a left-shift operation is an Undefined Behaviour in C.


  if (cli_api(cli,
        param, sizeof(param), 1024, /* Param, length, maxlen */
-       data, soffset, sizeof(data), /* data, length, maxlen */
+       data, soffset, data_size, /* data, length, maxlen */
        &rparam, &rprcnt,   /* return params, length */
        &rdata, &rdrcnt))   /* return data, length */
  {

Here data used to be a stack allocated array but was changed to a heap allocated buffer without updating the sizeof use.


   goto query;
  }
 
- if ((p->auth.auth_type != DCERPC_AUTH_TYPE_NTLMSSP) ||
-     (p->auth.auth_type != DCERPC_AUTH_TYPE_KRB5) ||
-     (p->auth.auth_type != DCERPC_AUTH_TYPE_SPNEGO)) {
+ if (!((p->auth.auth_type == DCERPC_AUTH_TYPE_NTLMSSP) ||
+       (p->auth.auth_type == DCERPC_AUTH_TYPE_KRB5) ||
+       (p->auth.auth_type == DCERPC_AUTH_TYPE_SPNEGO))) {
   return NT_STATUS_ACCESS_DENIED;
  }

Prior to this fix, the condition was always true and the function always returned "access denied".


- Py_RETURN_NONE;
  talloc_free(frame);
+ Py_RETURN_NONE;
}

Py_RETURN_NONE is a macro that hides a return statement. In this python binding many functions were returning before freeing heap allocated memory. This problem was present in dozens of functions.


  int i;
- for (i=0;ARRAY_SIZE(results);i++) {
+ for (i=0;i<ARRAY_SIZE(results);i++) {
   if (results[i].res == res) return results[i].name;
  }
  return "*";

Here the for condition was always true.


 int create_unlink_tmp(const char *dir)
 {
+ if (!dir) {
+  dir = tmpdir();
+ }
+
  size_t len = strlen(dir);
  char fname[len+25];
  int fd;
  mode_t mask;
 
- if (!dir) {
-  dir = tmpdir();
- }
-

Here the dir pointer was used before the null-check.

Overall I'm really pleased with PVS-Studio and I would recommend it. Unfortunately it's not officially available on Linux. Although you can just contact them if you're interested it seems :)

2 commentaires:

  1. Thanks for your efforts! Great you could run the tool on GNU/Linux.

    Now I have a couple of questions:
    * What version of Samba have you analyzed? Git revision?
    * The compiler haven't reported any warnings even with -Wextra or similar? What compiler (and version) have you used. The svn trunk versions of gcc-6 and clang getting even more strict.
    * cppcheck and clang static analyzer haven't warned about these issues?
    * gcc's and clang's ubsan haven't reported undefined behavior?
    * coverity hasn't found these bugs?
    * How many real bugs have been found at all? For 2 million code lines it looks like very very few.
    * Have you submitted these fixes already?

    Regards,
    Andrey

    RépondreSupprimer
    Réponses
    1. >What version of Samba have you analyzed? Git revision?

      commit feac8bf I believe.

      >The compiler haven't reported any warnings even with -Wextra or similar? What compiler (and version) have you used. The svn trunk versions of gcc-6 and clang getting even more strict.

      I usually work on a small subset of the project and recompile only those files (and I pay attention there). The waf build script enables very specific warnings (no includes-all flags like -Wextra). I believe GCC6 found an indent error recently. I personally use whatever GCC version is available on my work machine, which happens to be 4.8.3 at the moment.

      >cppcheck and clang static analyzer haven't warned about these issues?

      I haven't tried cppcheck. I believe someone on the team already runs the clang analyser before on the codebase.

      >gcc's and clang's ubsan haven't reported undefined behavior?

      I just took a look when we have a developper option on the build script to enable the address sanitizer but that's it. I will look into adding support for the rest of UBSan, thanks for the hint!

      >coverity hasn't found these bugs?

      I believe the project is already coverity-checked. These bugs might have been caught but ignored/missed or simply went undetected.

      >How many real bugs have been found at all? For 2 million code lines it looks like very very few.

      I still havent worked through all the reports. The remaining level-1 warnings require more analyzing. But so far a dozen and mostly on code that is not that used or poorly tested (we have an extensive test suite for the most critical code paths).

      >Have you submitted these fixes already?

      Yes, all the issues mentioned in this post have been fixed. Every patch is sent to the samba-technical mailling-list where it has to be reviewed by 2 other core members
      to get merged. It's a slow process so I try to only submit actual fixes and with good explanations to not waste time. That means I usually avoid small possible cleanups found via PVS related warnings.

      I should add that PVS-Studio found several issues that are more complex to explain (or sensible) or harder to make sure they are actual issues, or harder to fix so I haven't mentioned them. Finally I have only looked at the level 1 report which is already pretty huge.

      Supprimer