С момента выхода публичной Linux-версии PVS-Studio, статья о повторной проверке ядра Linux оставалась лишь вопросом времени. Проект, который пишется профессионалами со всего мира, который используют большое количество людей в самых разных сферах, который регулярно проверяется и тестируется различными инструментами — проверка такого проекта будет серьёзным испытанием для любого статического анализатора. Какие ошибки удалось найти PVS-Studio в таких условиях?

Как проверяли

Мы уже проверяли ядро Linux. С тех пор многое изменилось — теперь проверка ОС так же проста и удобна, как проверка любого другого проекта:

pvs-studio-analyzer trace - make
pvs-studio-analyzer analyze -o /path/to/report.log -j8

Всего несколько месяцев ушло на адаптацию и тестирование анализатора в Linux, который до недавнего времени был доступен только для Windows. В этот раз проверить ядро оказалось на порядок проще.

Для проверки была взята версия 4.9-rc4 (commit bc33b0ca11e3df467777a4fa7639ba488c9d4911).

Для написания статьи просматривались только диагностики общего назначения 1-2 уровней. Стоит отметить высокое качество кода — плотность предупреждений, указывающих на реальные недоработки, крайне низкая.

Для статьи я выбирал те предупреждения, которые с наибольшей вероятностью указывают на настоящие ошибки/опечатки. Надо понимать, что помимо полезных предупреждений анализатор выдаёт и ложные срабатывания или выявляет плохо оформленный или «дурно пахнущий» код, который тем не менее не является по настоящему ошибочным.

К сожалению, количество ложных срабатываний под Linux больше, чем хотелось бы. Думаю, это связно с молодостью версии анализатора, предназначенной для Linux систем. Мы проделали и продолжаем делать огромную работу по минимизации количества ложных срабатываний. Код Linux помог нам стать лучше и внести множество полезных изменений в анализатор, и теперь хотелось бы ответить взаимностью.

Опечатки

Самая распространённая категория ошибок связана с обычными опечатками и Copy-Paste. Если вы читали наши статьи раньше, я думаю, вы уже в этом убедились. Они появляются в любых проектах на любых операционных системах на любых языках. В таких ошибках и раскрывается потенциал статического анализатора: другими инструментами найти их значительно сложнее. Посмотрим, как с ними обстоят дела в ядре Linux:

Предупреждение PVS-Studio: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 2384, 2390. debug.c 2390

int dbg_check_nondata_nodes_order(....)
{
  ....
  sa = container_of(cur, struct ubifs_scan_node, list);
  sb = container_of(cur->next, struct ubifs_scan_node, list);

  if (sa->type != UBIFS_INO_NODE && sa->type != UBIFS_DENT_NODE &&
      sa->type != UBIFS_XENT_NODE) {
    ubifs_err(c, "bad node type %d", sa->type);
    ubifs_dump_node(c, sa->node);
    return -EINVAL;
  }
  if (sa->type != UBIFS_INO_NODE && sa->type != UBIFS_DENT_NODE &&
      sa->type != UBIFS_XENT_NODE) {
    ubifs_err(c, "bad node type %d", sb->type);
    ubifs_dump_node(c, sb->node);
    return -EINVAL;
  }
  ....
}

Анализатор жалуется на два одинаковых условия подряд: видимо, во втором забыли поменять sa на sb. Ну и кто после этого скажет, что в крутых проектах не копипастят?

Предупреждение PVS-Studio: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the first argument. spectral.c 341

static ssize_t write_file_spec_scan_ctl(struct file *file,
          const char __user *user_buf,
          size_t count, loff_t *ppos)
{
  struct ath10k *ar = file->private_data;
  char buf[32];
  ssize_t len;
  int res;

  len = min(count, sizeof(buf) - 1);
  if (copy_from_user(buf, user_buf, len))
    return -EFAULT;

  buf[len] = '\0';

  mutex_lock(&ar->conf_mutex);

  if (strncmp("trigger", buf, 7) == 0) {
    ....
  } else if (strncmp("background", buf, 9) == 0) {
    res = ath10k_spectral_scan_config(ar, SPECTRAL_BACKGROUND);
  } else if (strncmp("manual", buf, 6) == 0) {
    res = ath10k_spectral_scan_config(ar, SPECTRAL_MANUAL);
  } else if (strncmp("disable", buf, 7) == 0) {
    res = ath10k_spectral_scan_config(ar, SPECTRAL_DISABLED);
  } else {
    res = -EINVAL;
  }

  mutex_unlock(&ar->conf_mutex);

  if (res < 0)
    return res;

  return count;
}

Классический вид ошибки: в функцию нужно передать два аргумента: указатель на строку и её длину. Часто, когда аргументом служит литерал, длину считать ленятся и пишут просто число. В дело вступает человеческий фактор: ошибаются часто.

Смотрите, в коде есть несколько подряд strncmp. В каждый из них передают литерал. И в strncmp(«background», buf, 9) длину рассчитали неверно: слово «background» состоит из 10, а не из 9 символов.

Предупреждение PVS-Studio: V666 Consider inspecting third argument of the function 'memcpy'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. dpt_i2o.c 403

static void adpt_inquiry(adpt_hba* pHba)
{
  ....
  memset(pHba->detail, 0, sizeof(pHba->detail));
  memcpy(&(pHba->detail), "Vendor: Adaptec ", 16);
  memcpy(&(pHba->detail[16]), " Model: ", 8);
  memcpy(&(pHba->detail[24]), (u8*) &buf[16], 16);
  memcpy(&(pHba->detail[40]), " FW: ", 4);         // <=
  memcpy(&(pHba->detail[44]), (u8*) &buf[32], 4);
  pHba->detail[48] = '\0';  /* precautionary */
  ....
}

Ещё один пример. Длина строки " FW: " равна 5, а вовсе не 4 символам.

Как избавиться от такой ошибки? В C можно использовать макрос наподобие:

#define str_len(S) (sizeof(S) / sizeof((S)[0]))

Но использование таких макросов само по себе опасно: лучше конечно добавить compiler-specific проверок на то, что переданный аргумент действительно массив.

Для читателей, пишущих на C++, могу порекомендовать std::string_view, который наконец-то появился в C++17. Лучше не передавать в функцию строки парой указатель-длина. Но если нужно вручную посчитать размер массива (например, чтобы передать его в функцию memcpy), то можно использовать std::size(array) или его аналог: для литералов размер будет посчитан в compile-time.

Избегайте повторения кода и не ленитесь использовать средства языка (будь то макросы или шаблоны) для compile time вычислений!

Предупреждение PVS-Studio: V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: «30min» «No timeout». lp8788-charger.c 657

static ssize_t lp8788_show_eoc_time(struct device *dev,
        struct device_attribute *attr, char *buf)
{
  struct lp8788_charger *pchg = dev_get_drvdata(dev);
  char *stime[] = { "400ms", "5min", "10min", "15min",
      "20min", "25min", "30min" "No timeout" };
  ....
}

Как известно, два записанных подряд литерала конкатенируются. Это позволяет их удобно использовать, например, в макросах. Опасность возникает, когда мы пишем массив из таких литералов: можно пропустить запятую и получить неожиданный результат.

В данном случае «слипнутся» два последних литерала и получится «30minNo timeout». Это двойная ошибка. Во-первых, текст неправильный, во-вторых в массиве будет не хватать одного элемента, что может привести к выходу за границу массива.

Советую использовать другой способ форматирования, в нём такая ошибка станет заметной:

char *stime[] = {
    "400ms"
  , "5min"
  , "10min"
  , "15min"
  , "20min"
  , "25min"
  , "30min"
    "No timeout"
};

Подробнее о таком способе табличного оформления кода писал мой коллега Андрей Карпов. Предлагаю ознакомиться с главой N13 из его небольшой книги.

Предупреждение PVS-Studio: V764 Possible incorrect order of arguments passed to 'ahc_9005_subdevinfo_valid' function: 'device' and 'vendor'. aic7xxx_pci.c 695

const struct ahc_pci_identity *
ahc_find_pci_device(ahc_dev_softc_t pci)
{
  ....
  if (ahc_get_pci_function(pci) > 0
   && ahc_9005_subdevinfo_valid(device, vendor, // <=
                                subdevice, subvendor)
   && SUBID_9005_MFUNCENB(subdevice) == 0)
    return (NULL);

  ....
}

Иногда бывает сложно понять, на что ругается анализатор. Кстати, такое часто бывает: человек не понял, что ему написал анализатор, отправил нам отчёт с «ложным срабатыванием», а там на самом деле ошибка. Вот и мне здесь показалось, что это ложное срабатывание: функция определена немного выше по коду и там все параметры на своих местах. Вот как она выглядит:

static int
ahc_9005_subdevinfo_valid(uint16_t device, uint16_t vendor,
        uint16_t subdevice, uint16_t subvendor)
{
  ....
}

В чём же дело? Оказывается, что ещё выше есть объявление этой функции и вот там-то эти аргументы и перепутаны. По факту ничего страшного в логике программы нет, но лучше всё-таки поправить, дабы никого не смущать и не сбивать с толку.

static int ahc_9005_subdevinfo_valid(uint16_t vendor, uint16_t device,
    uint16_t subvendor, uint16_t subdevice);

Но что самое забавное, ошибка здесь уже была: параметры действительно были перепутаны, просто забыли поправить объявление. Хорошо, что анализатор тоже нашёл это место.

Предупреждение PVS-Studio: V549 The first argument of 'memcpy' function is equal to the second argument. wilc_wfi_cfgoperations.c 1345

static int del_pmksa(struct wiphy *wiphy,
                     struct net_device *netdev,
                     struct cfg80211_pmksa *pmksa)
{
  ....
  for (; i < (priv->pmkid_list.numpmkid - 1); i++) {
    memcpy(priv->pmkid_list.pmkidlist[i].bssid,
           priv->pmkid_list.pmkidlist[i + 1].bssid,
           ETH_ALEN);
    memcpy(priv->pmkid_list.pmkidlist[i].pmkid,
           priv->pmkid_list.pmkidlist[i].pmkid,
           PMKID_LEN);
  }
  ....
}

В последнем memcpy совпадают указатели. Возможно, хотели написать по аналогии с предыдущим выражением:

memcpy(priv->pmkid_list.pmkidlist[i].pmkid,
       priv->pmkid_list.pmkidlist[i + 1].pmkid,
       PMKID_LEN);

Неиспользуемые переменные

Предупреждение PVS-Studio: V575 The 'strncasecmp' function processes '0' elements. Inspect the third argument. linux_wlan.c 1121

static int mac_ioctl(struct net_device *ndev,
                     struct ifreq *req,
                     int cmd)
{
  u8 *buff = NULL;
  s8 rssi;
  u32 size = 0, length = 0;
  struct wilc_vif *vif;
  s32 ret = 0;
  struct wilc *wilc;

  vif = netdev_priv(ndev);
  wilc = vif->wilc;

  if (!wilc->initialized)
    return 0;

  switch (cmd) {
  case SIOCSIWPRIV:
  {
    struct iwreq *wrq = (struct iwreq *)req;

    size = wrq->u.data.length;

    if (size && wrq->u.data.pointer) {
      buff = memdup_user(wrq->u.data.pointer,
                         wrq->u.data.length);
      if (IS_ERR(buff))
        return PTR_ERR(buff);

      if (strncasecmp(buff, "RSSI", length) == 0) { // <=
        ....
      }
    }
  }
  ....
  }

done:

  kfree(buff);

  return ret;
}

В функцию strncasecmp в качестве аргумента длины передали 0. В коде нет места, где бы изменялась переменная length, поэтому её значение останется нулём. Наверное, нужно было использовать size.

Предупреждение PVS-Studio: V751 Parameter 'LCDheight' is not used inside function body. init.c 339

static
unsigned short
SiS_GetModeID(int VGAEngine, unsigned int VBFlags,
              int HDisplay, int VDisplay,
              int Depth, bool FSTN,
              int LCDwidth, int LCDheight)
{
  unsigned short ModeIndex = 0;

  switch(HDisplay)
  {
  case 320:
    if(VDisplay == 200) ModeIndex = ModeIndex_320x200[Depth];
    else if(VDisplay == 240) {
      if((VBFlags & CRT2_LCD) && (FSTN))
        ModeIndex = ModeIndex_320x240_FSTN[Depth];
      else
        ModeIndex = ModeIndex_320x240[Depth];
    }
    break;
  case 400:
    if((!(VBFlags & CRT1_LCDA)) ||
       ((LCDwidth >= 800) && (LCDwidth >= 600))) { // <=
      if(VDisplay == 300) ModeIndex = ModeIndex_400x300[Depth];
    }
    break;
  case 512:
    if((!(VBFlags & CRT1_LCDA)) ||
       ((LCDwidth >= 1024) && (LCDwidth >= 768))) { // <=
      if(VDisplay == 384) ModeIndex = ModeIndex_512x384[Depth];
    }
    break;
    ....
  }

  return ModeIndex;
}

Не всегда неиспользуемые в функции параметры — это ошибка. В достаточно старых API возникают ситуации, когда параметр стал не нужен и его перезаписывают или просто не используют. Но присмотритесь внимательнее к этому фрагменту: здесь забыли сравнить высоту. Вместо этого появились сравнения вида '(A > 5) && (A > 3)', которые сами по себе избыточны.

Путаница в приоритетах операций

Предупреждение PVS-Studio: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. core.c 1046

static int nvme_pr_preempt(struct block_device *bdev,
                           u64 old, u64 new,
                           enum pr_type type, bool abort)
{
  u32 cdw10 = nvme_pr_type(type) << 8 | abort ? 2 : 1;
  return nvme_pr_command(bdev, cdw10, old, new,
                         nvme_cmd_resv_acquire);
}

Тернарный оператор в C — это очень опасный оператор. Не просто так одна из первых диагностик общего назначения в PVS-Studio посвящена ему. Дело в том, что у него низкий приоритет, и в сложных выражениях легко запутаться и получить совершенно иной порядок вычислений. Поэтому лучше, когда сомневаешься, использовать скобки.

Подозрительные проверки

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 375, 377. trx.c 375

bool rtl92ee_rx_query_desc(struct ieee80211_hw *hw,
         struct rtl_stats *status,
         struct ieee80211_rx_status *rx_status,
         u8 *pdesc, struct sk_buff *skb)
{
  struct rtl_priv *rtlpriv = rtl_priv(hw);
  struct rx_fwinfo *p_drvinfo;
  struct ieee80211_hdr *hdr;
  u32 phystatus = GET_RX_DESC_PHYST(pdesc);

  ....

  status->macid = GET_RX_DESC_MACID(pdesc);
  if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
    status->wake_match = BIT(2);
  else if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
    status->wake_match = BIT(1);
  else if (GET_RX_STATUS_DESC_UNICAST_MATCH(pdesc))
    status->wake_match = BIT(0);
  else
    status->wake_match = 0;
  
  ....
}

Со стороны сложно понять, что здесь не так. Два раза идёт одна и та же проверка макросом GET_RX_STATUS_DESC_MAGIC_MATCH. Если посмотрим его объявление, то увидим два других макроса:

#define GET_RX_STATUS_DESC_PATTERN_MATCH(__pdesc)
        LE_BITS_TO_4BYTE(__pdesc+12, 29, 1)
#define GET_RX_STATUS_DESC_UNICAST_MATCH(__pdesc)
        LE_BITS_TO_4BYTE(__pdesc+12, 30, 1)
#define GET_RX_STATUS_DESC_MAGIC_MATCH(__pdesc)
        LE_BITS_TO_4BYTE(__pdesc+12, 31, 1)

Возможно хотели использовать отсутствующий в исходном фрагменте GET_RX_STATUS_DESC_PATTERN_MATCH. Иначе эта проверка просто бессмысленна.

Предупреждение PVS-Studio: V547 Expression '(ptr[3] & 0x1E) != 0x03' is always true. sd.c 4115

int ext_sd_send_cmd_get_rsp(struct rtsx_chip *chip,
    u8 cmd_idx, u32 arg, u8 rsp_type,
    u8 *rsp, int rsp_len, bool special_check)
{
  int retval;
  int timeout = 100;
  u16 reg_addr;
  u8 *ptr;

  ....

  if (cmd_idx == SELECT_CARD) {
    if (rsp_type == SD_RSP_TYPE_R2) {
      if ((ptr[3] & 0x1E) != 0x04) {
        rtsx_trace(chip);
        return STATUS_FAIL;
      }

    } else if (rsp_type == SD_RSP_TYPE_R0) {
      if ((ptr[3] & 0x1E) != 0x03) {           // <=
        rtsx_trace(chip);
        return STATUS_FAIL;
      }
    }
  }

  ....
}

Ошибка связана с битовыми операциями. Результат побитовой конъюнкции с 0x1E из-за одного бита никогда не будет равен значению 0x03:

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1277, 1282. ks_wlan_net.c 1277

static int ks_wlan_set_power(struct net_device *dev,
           struct iw_request_info *info,
           struct iw_param *vwrq, char *extra)
{
  struct ks_wlan_private *priv =
      (struct ks_wlan_private *)netdev_priv(dev);
  short enabled;

  if (priv->sleep_mode == SLP_SLEEP) {
    return -EPERM;
  }
  /* for SLEEP MODE */
  enabled = vwrq->disabled ? 0 : 1;
  if (enabled == 0) {  /* 0 */
    priv->reg.powermgt = POWMGT_ACTIVE_MODE;
  } else if (enabled) {  /* 1 */
    if (priv->reg.operation_mode == MODE_INFRASTRUCTURE)
      priv->reg.powermgt = POWMGT_SAVE1_MODE;
    else
      return -EINVAL;
  } else if (enabled) {  /* 2 */
    if (priv->reg.operation_mode == MODE_INFRASTRUCTURE)
      priv->reg.powermgt = POWMGT_SAVE2_MODE;
    else
      return -EINVAL;
  } else
    return -EINVAL;

  hostif_sme_enqueue(priv, SME_POW_MNGMT_REQUEST);

  return 0;
}

Сократим пример до:

enabled = vwrq->disabled ? 0 : 1;
if (enabled == 0)   {  /* 0 */
  ....
} else if (enabled) {  /* 1 */
  ....
} else if (enabled) {  /* 2 */
  ....
} else
  ....

Данный код выглядит очень странно. Вроде бы область значений чётко обговорена выражением выше: enabled равен 0 или 1. Но проверяется целых 4 значения. При этом комментарии только мешают: если цифры должны были обозначать возможное значение переменной, то сейчас они не соответствуют действительности: проверка на 1 и 2 записаны одинаковым образом.

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 422, 424. Hal8188ERateAdaptive.c 422

static int odm_ARFBRefresh_8188E(
        struct odm_dm_struct *dm_odm,
        struct odm_ra_info *pRaInfo)
{  /*  Wilson 2011/10/26 */
  ....
  if (pRaInfo->HighestRate > 0x13)
    pRaInfo->PTModeSS = 3;
  else if (pRaInfo->HighestRate > 0x0b)
    pRaInfo->PTModeSS = 2;
  else if (pRaInfo->HighestRate > 0x0b)
    pRaInfo->PTModeSS = 1;
  else
    pRaInfo->PTModeSS = 0;
  ....
  return 0;
}

Ещё одно место, в котором идут подряд два условия. Обратите внимание, что тела при этом разные. Сложно сказать, есть ли здесь реальная ошибка, или это просто неиспользуемый код: это уже задача разработчиков проекта. Задача анализатора обратить внимание на подозрительное место.

Предупреждение PVS-Studio: V734 An excessive check. Examine the conditions containing search for the substrings «interleaver» and «deinterleaver». sst-atom-controls.c 1449

static int sst_fill_widget_module_info(
  struct snd_soc_dapm_widget *w,
  struct snd_soc_platform *platform)
{
  struct snd_kcontrol *kctl;
  int index, ret = 0;
  struct snd_card *card = platform->component.card->snd_card;
  char *idx;

  down_read(&card->controls_rwsem);

  list_for_each_entry(kctl, &card->controls, list) {
    ....

    } else if (strstr(kctl->id.name, "interleaver")) {
      struct sst_enum *e = (void *)kctl->private_value;

      e->w = w;

    } else if (strstr(kctl->id.name, "deinterleaver")) {
      struct sst_enum *e = (void *)kctl->private_value;

      e->w = w;
    }

    ....
  }

  up_read(&card->controls_rwsem);
  return 0;
}

В этом фрагменте последовательно проверяют наличие нескольких подстрок в одной строке. Для наглядности я оставил только интересующие нас подстроки. Предположим, что мы не нашли interleaver — тогда нет смысла искать deinterleaver, ведь подстроки interleaver уже точно нет. Поэтому этот участок кода никогда не заработает, но, так как тела у if и else одинаковые, это не страшно. Это просто избыточный код.

Предупреждение PVS-Studio: V547 Expression 'block' is always true. svclock.c 873

void
nlmsvc_grant_reply(struct nlm_cookie *cookie, __be32 status)
{
  struct nlm_block  *block;

  dprintk("grant_reply: looking for cookie %x, s=%d \n",
    *(unsigned int *)(cookie->data), status);
  if (!(block = nlmsvc_find_block(cookie)))
    return;

  if (block) {
    if (status == nlm_lck_denied_grace_period) {
      /* Try again in a couple of seconds */
      nlmsvc_insert_block(block, 10 * HZ);
    } else {
      /* Lock is now held by client, or has been rejected.
       * In both cases, the block should be removed. */
      nlmsvc_unlink_block(block);
    }
  }
  nlmsvc_release_block(block);
}

Этот пример демонстрирует, почему статическому анализатору недостаточно выполнять Pattern-based analysis, обходя AST. Важно уметь выполнять Control flow analysis и Data flow analysis. В момент, когда block == NULL происходит return, соответственно дальше по коду мы точно можем сказать, что указатель ненулевой. И когда мы встречаем проверку на NULL, мы точно понимаем, что что-то тут не так.

Похоже, что вторая проверка указателя здесь просто лишняя. Однако, вдруг здесь хотели проверить другую переменную? Кто знает… Этот код анализатору явно стоит предоставить разработчику для проверки.

Аналогичная ситуация:

Предупреждение PVS-Studio: V547 Expression 'sym' is always true. menu.c 498

bool menu_is_visible(struct menu *menu)
{
  struct menu *child;
  struct symbol *sym;

  ....

  if (!sym || sym_get_tristate_value(menu->sym) == no) // <=
    return false;

  for (child = menu->list; child; child = child->next) {
    if (menu_is_visible(child)) {
      if (sym)                                         // <=
        sym->flags |= SYMBOL_DEF_USER;
      return true;
    }
  }

  return false;
}

Ошибка в макросе

Предупреждение PVS-Studio: V733 It is possible that macro expansion resulted in incorrect evaluation order. Check expression: request->rq_timeout + 5 * 1000. niobuf.c 637

#define CFS_FAIL_TIMEOUT(id, secs) cfs_fail_timeout_set(id, 0, secs * 1000, CFS_FAIL_LOC_NOSET)

#define OBD_FAIL_TIMEOUT(id, secs) CFS_FAIL_TIMEOUT(id, secs)

int ptl_send_rpc(struct ptlrpc_request *request, int noreply)
{
  ....
  OBD_FAIL_TIMEOUT(OBD_FAIL_PTLRPC_DELAY_SEND,
                   request->rq_timeout + 5);
  ....
}

А вот такие ошибки очень редки. До этого я видел только одно срабатывание этой диагностики в реальном проекте: примечательно, что это был FreeBSD. Ошибку допустили в определении макроса: лучше всего все его параметры окружать скобками. Если этого не делать, то возможен такой случай: при подстановке 'x + 5' в 'secs * 1000' получается 'x + 5 * 1000', а это явно не то, что ожидал автор.

Бессмысленный memset

Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'ps' buffer. The memset_s() function should be used to erase the private data. atom.c 1383

int amdgpu_atom_asic_init(struct atom_context *ctx)
{
  int hwi = CU16(ctx->data_table + ATOM_DATA_FWI_PTR);
  uint32_t ps[16];
  int ret;

  memset(ps, 0, 64);

  ps[0] = cpu_to_le32(CU32(hwi + ATOM_FWI_DEFSCLK_PTR));
  ps[1] = cpu_to_le32(CU32(hwi + ATOM_FWI_DEFMCLK_PTR));
  if (!ps[0] || !ps[1])
    return 1;

  if (!CU16(ctx->cmd_table + 4 + 2 * ATOM_CMD_INIT))
    return 1;
  ret = amdgpu_atom_execute_table(ctx, ATOM_CMD_INIT, ps);
  if (ret)
    return ret;

  memset(ps, 0, 64); // <=

  return ret;
}

Нет смысла добавлять memset перед return: компилятор, увидев, что эта операция не меняет видимое состояние программы (массив всё равно выходит из области видимости), удалит её. Если нужно стереть какие-то важные данные, то для этого стоит использовать memset_s или написать свой аналог.

Это ошибка кстати фактически является уязвимостью. Не затираются данные, которые должны быть затёрты. С подробностями можно познакомиться в описании диагностики V597. Кстати, это очень распространённая уязвимость: proof.

Опасное использование memcmp

Предупреждение PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. host.c 1789

static void power_control_timeout(unsigned long data)
{
  ....
  u8 other = memcmp(requester->frame_rcvd.iaf.sas_addr,
                    iphy->frame_rcvd.iaf.sas_addr,
                    sizeof(requester->frame_rcvd.iaf.sas_addr));

  if (other == 0) {
    ....
  }
  ....
}

Если внимательно прочитать, что говорит документация о возвращаемом значении memcmp, то мы увидим, что гарантии о каком-либо конкретном диапазоне нет: функция может вернуть любое число в рамках своего типа. И это не всегда -1, 0 и 1. Поэтому нельзя сохранять его значение в переменной меньшего типа: при потере старших разрядов младшие могут составить ноль. Похожая ошибка привела к уязвимости в MySQL/MariaDB.

Заключение

Как уже говорилось, Linux — очень качественный, хорошо протестированный проект. Найти в нём ошибку, даже самую незначительную, — это уже повод для гордости. Это же и повод задуматься о том, сколько ошибок можно было бы найти до отладки и тестирования: именно в таком амплуа ценен статический анализ. Вы можете в этом убедиться, попробовав PVS-Studio. Для линукса вы можете получить триал-версию анализатора, написав нам на почту. А для своих некоммерческих проектов можно использовать PVS-Studio бесплатно: для этого достаточно познакомиться с этой статьёй и воспользоваться открытой и свободной утилитой how-to-use-pvs-studio-free.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Pavel Belikov. Linux Kernel, tested by the Linux-version of PVS-Studio