Баги в коде — явление нередкое, но сегодня мы исследуем не просто ошибки, а настоящие космические баги! Что скрывает проект, созданный в недрах NASA? Готовьте свои шапочки из фольги!

WorldWind Java — это открытая программная платформа, разработанная NASA для визуализации геопространственных данных в 3D-пространстве. Проект реализован на языке Java и предоставляет разработчикам инструменты для создания интерактивных приложений, отображающих спутниковые снимки, цифровые модели рельефа, географические аннотации и другие пространственные слои. Благодаря WorldWind Java можно разрабатывать как научные, так и прикладные решения в области геоинформационных систем, моделирования окружающей среды, обучения и визуализации данных.

В этой статье посмотрим на то, какие баги нашлись в исходном коде проекта с помощью статического анализатора PVS-Studio.

Примечание. Этот же проект, но на C#, мы проверяли ещё в далёком 2016 году. Прочитать статью можно здесь.

Классический NullPointerException

По традиции начнём с того, как разработчики пытаются разыменовать ничего.

Фрагмент 1

....
this.trackLayers.remove(track);
this.wwd.getModel().getLayers().remove(layer);     <=
if (this.wwd != null) {                            <=
    this.wwd.redraw();
}
....

Предупреждение PVS-Studio: V6060 The 'this.wwd' reference was utilized before it was verified against null. TrackController.java 229

Итак, действительно классика. Свойство wwd проверяется на существование после того, как оно было использовано. То есть разработчики допускают вероятность, что это свойство может быть нулевым и даже обрабатывается так, как нужно, только вот не всё, что нужно.

Подобная ситуация происходит и в следующем фрагменте.

Фрагмент 2

public ProjectionTransverseMercator(Angle centralMeridian)
{
  super(makeProjectionLimits(centralMeridian, DEFAULT_WIDTH));   <=
  if (centralMeridian == null)                                   <=
  {
    ....
  }
  this.centralMeridian = centralMeridian;
}

Предупреждение PVS-Studio: V6060 The 'centralMeridian' reference was utilized before it was verified against null. ProjectionTransverseMercator.java 72

Здесь, правда, ситуация слегка поинтереснее: объект используется внутри загадочного метода makeProjectionLimits. Заглянем в него:

protected static Sector makeProjectionLimits(
  Angle centralMeridian, 
  Angle width)
{
  double minLon = centralMeridian.degrees - width.degrees;
  ....
}

Здесь объект centralMeridian разыменовывается буквально первым действием. Поэтому перед вызовом этого метода стоило проверить, что centralMeridian не равен null.

Охотники за приведениями типов

Интригующий заголовок, согласитесь? Предлагаю сразу посмотреть на фрагмент.

Фрагмент 3

protected short outlineStipplePattern;
....
public BasicShapeAttributes() {
  ....
  this.outlineWidth = 1;
  this.outlineStippleFactor = 0;
  this.outlineStipplePattern = (short) 0xF0F0;    <=
  this.imageSource = null;
  this.imageScale = 1;
  ....
}
....

Предупреждение PVS-Studio: V6124 Converting an integer literal to the type with a smaller value range will result in overflow. BasicShapeAttributes.java 105

Под определённый целочисленный тип выделяется конечное количество байт. Если значение выходит за рамки выделенного количества байт, то лишние биты у значения... просто отсекаются. При этом код компилируется и вполне успешно работает. Только вот значение будет уже не тем, на которое мы рассчитывали.

В этом фрагменте мы пытаемся присвоить число 61680 в тип short, где максимальное значение 32767. Это приводит к переполнению.

Подобных срабатываний в этом проекте было довольно много. Приведу ещё один пример.

Фрагмент 4

....
final int BAND_Y = 0, BAND_ALPHA = 1;

final int ALPHA_OPAQUE = (short) 0xFFFF;     <=
final int ALPHA_TRANSLUCENT = (short) 0;
....

Предупреждение PVS-Studio: V6124 Converting an integer literal to the type with a smaller value range will result in overflow. ImageUtil.java 2085

В результате выполнения этой строчки кода в переменной ALPHA_OPAQUE мы получим значение -1. Теперь проведём небольшую манипуляцию и поменяем в приведении тип на int:

....
final int BAND_Y = 0, BAND_ALPHA = 1;

final int ALPHA_OPAQUE = (int) 0xFFFF;     <=
final int ALPHA_TRANSLUCENT = (short) 0;
....

В таком варианте мы уже получим значение 65535. Возможно, авторы хотели сделать какой-то фокус с помощью приведения типов, но всё же мне кажется, что в итоге они переиграли сами себя.

Фрагмент 5

....
protected static final int UTM_MAX_LATITUDE = 84;
....
double northLat = UTM_MAX_LATITUDE / 2;
....

Предупреждение PVS-Studio: V6094 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. UTMGraticuleLayer.java 459

Ещё одна ошибка с приведением типов. В этот раз мы делим одно целое число на другое целое число и записываем результат в переменную с типом double. То есть мы полагаем, что в результате деления будет число с плавающей запятой. На самом деле в результате деления мы получим тоже целое число, а уже потом оно будет приведено к типу double.

Чтобы поправить эту ситуацию и избежать внезапного исчезновения дробной части, нам нужно всего лишь добавить явное приведение к типу double:

....
protected static final int UTM_MAX_LATITUDE = 84;
....
double northLat = (double)UTM_MAX_LATITUDE / 2;
....

Безусловно

Итак, что же могло пойти не так в использовании условных операторов? Ну например, выбор без выбора.

Фрагмент 6

....
final double CENTER_START = this.useMidZoom ? 0.2 : 0.0;
final double CENTER_STOP = this.useMidZoom ? 0.8 : 0.8;     <=
....

Предупреждение PVS-Studio: V6012 The '?:' operator, regardless of its conditional expression, always returns one and the same value '0.8'. KMLOrbitViewController.java 287

Хочется включить синдром поиска глубинного смысла и понять, зачем это было сделано. Но, думаю, здесь это излишне. Интересно, что этот фрагмент кода был целиком скопирован в другой метод, и там анализатор выдал то же самое предупреждение.

Фрагмент 7

if (Southern_Hemisphere != 0)
{
  Easting = -(rho * Math.sin(dlam) - Polar_False_Easting); 
  Northing = rho * Math.cos(dlam) + Polar_False_Northing;
} else
  Easting = rho * Math.sin(dlam) + Polar_False_Easting;
  Northing = -rho * Math.cos(dlam) + Polar_False_Northing;
....

Предупреждение PVS-Studio: V6040 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. PolarCoordConverter.java 349

Python код в Java заказывали? В else ветке этого ветвления авторы забыли поставить фигурные скобки. Видимо, предполагая, что всё отработает так, как должно. Однако вторая строка else ветки, в которой присваивается переменная Northing, будет выполняться вне зависимости от результата условия, поэтому значение этой переменной может быть неверным.

Фрагмент 8

....
if (altitude == null || altitude == 0)
{
  double t = sector.getDeltaLonRadians() > 
             sector.getDeltaLonRadians() ? 
             sector.getDeltaLonRadians() :
             sector.getDeltaLonRadians();     <=
  ....
}
....

Предупреждение PVS-Studio: V6012 The '?:' operator, regardless of its conditional expression, always returns one and the same value 'sector.getDeltaLonRadians()'. GazetteerPanel.java 328

Здесь уже всё запутаннее, чем в предыдущем фрагменте. Вызов одного и того же метода сравнивается с собой же, а результатом в if и else ветках является опять же вызов этого метода sector.getDeltaLonRadians.

Однако здесь всё тоже довольно прозаично. У объекта типа Sector есть методы getDeltaLonRadians и getDeltaLatRadians, которые соответственно возвращают дельты широты и долготы в радианах. И авторы просто перепутали названия методов, из-за чего тернарный оператор абсолютно потерял смысл.

Поправить можно было бы вот так:

....
if (altitude == null || altitude == 0)
{
  double t = sector.getDeltaLonRadians() > 
             sector.getDeltaLatRadians() ? 
             sector.getDeltaLonRadians() : 
             sector.getDeltaLatRadians();
  ....
}
....

Причём такая ошибка в проекте не одна!

Фрагмент 9

....
if (MGRS.getLatitude().degrees != 0 || 
    MGRS.getLatitude().degrees != 0)
{
  lat = MGRS.getLatitude();
  lon = MGRS.getLongitude();
}
else
  return null;
....

Предупреждение PVS-Studio: V6001 There are identical sub-expressions 'MGRS.getLatitude().degrees != 0' to the left and to the right of the '||' operator. GoToCoordinatePanel.java 185

Широкую делайте! Широкую на широкую!..

Неизвестный философ

Да, здесь авторы снова перепутали широту и долготу, поэтому одно и то же условие повторяется дважды, о чём и сообщил анализатор.

Фрагмент 10

....
if (this.currentResourceTile != null)
{
  if (this.currentResourceTile.getLevelNumber() == 0 && 
      this.forceLevelZeroLoads &&
      !this.currentResourceTile.isTextureInMemory(
        dc.getTextureCache()) &&      <=
      !this.currentResourceTile.isTextureInMemory(
        dc.getTextureCache()          <=
      )
  ) 
  this.forceTextureLoad(this.currentResourceTile);
....

Предупреждение PVS-Studio: V6001 There are identical sub-expressions to the left and to the right of the '&&' operator. TiledImageLayer.java 478

Помимо предупреждения, здесь в целом комментарии излишни. С обеих сторон от оператора && стоит одно и то же условие. Возможно, здесь должен был быть другой метод, как в предыдущем фрагменте, однако методов с похожим названием я не нашёл.

Зато нашёлся ещё один похожий фрагмент.

Фрагмент 11

....
if (this.currentResourceTile != null)
{
  if (this.currentResourceTile.getLevelNumber() == 0 && 
      this.forceLevelZeroLoads &&
      !this.currentResourceTile.isTextureInMemory(
        dc.getTextureCache()) &&     <=
      !this.currentResourceTile.isTextureInMemory(
        dc.getTextureCache()         <=
      )
  ) 
  this.forceTextureLoad(this.currentResourceTile);
....

Предупреждение PVS-Studio: V6001 There are identical sub-expressions to the left and to the right of the '&&' operator. MercatorTiledImageLayer.java 399

У вас никогда не было дежавю? Да, это абсолютно тот же самый фрагмент кода, который ровно в таком же виде был скопирован в другую часть проекта. Если быть точнее, то в другую реализацию интерфейса AbstractLayer.

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

Но последний ли это фрагмент, где что-то скопировали?

Бешеный принтер

Ох, вы даже не представляете, что может случиться после того, как вы нажали заветные Ctrl + C и Ctrl + V...

Фрагмент 12

if (line.startsWith("LINE "))
....
else if (line.startsWith("EDGEVIS "))
  this.edgeVis = getStringValue(line);
else if (line.startsWith("COLRTABLE "))
  this.colorTable = getIntegerValues(line);
else if (line.startsWith("COLRTABLE "))
  this.colorTable = getIntegerValues(line);
else if (line.startsWith("SCALEMODE "))
  this.scale = getScaleValue(line);

Предупреждение PVS-Studio: V6003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. GeoSymAttributeConverter.java 151

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

Фрагмент 13

....
switch (control) {
  case NORTH:
    azimuth = Angle.ZERO;
    break;
  ....
  case NORTH_LEADER:
    azimuth = Angle.ZERO;
    break;
  default:
    break;
}
....

Предупреждение PVS-Studio: V6067 Two or more case-branches perform the same actions. MeasureTool.java 1601

В этом switch–case в двух случаях выполняется один и тот же код. Вероятно, так и должно быть, но тогда нужно объединить эти два условия, дабы избежать повторения кода:

....
switch (control) {
  case NORTH:
  case NORTH_LEADER:
    azimuth = Angle.ZERO;
    break;
  .... 
  default:
    break;
}
....

Однако также вероятно, что поведение для NORTH_LEADER должно было отличаться от поведения для NORTH.

Фрагмент 14

public final URL getUrl()
{
  return url;
}
....
public final URL getURL()
{
  return this.url;
}

Предупреждение PVS-Studio: V6032 It is odd that the body of method 'getUrl' is fully equivalent to the body of another method 'getURL'. URLRetriever.java 123

Я уже говорил тебе, что такое безумие? Безумие — это точное повторение одного и того же действия, раз за разом, в надежде на изменение.

Ваас Монтенегро, Far Cry 3

Да, эти методы похожи как две капли воды. Но в них есть всё-таки небольшая разница: второй вариант обращается к url через this, а также URL в его названии написано большими буквами. Всё!

Ещё большая загадка этих методов в том, что они не используются. То есть мы не просто написали уже существующий метод второй раз, но ещё и нигде его не используем :)

Фрагмент 15

public void beginPolyline()
{
  this.lastIndex = -1;
}

public void endPolyline()
{
  this.lastIndex = -1;
}

Предупреждение PVS-Studio: V6032 It is odd that the body of method 'beginPolyline' is fully equivalent to the body of another method 'endPolyline'. PolylineTessellator.java 57

Итак, первый метод записывает в lastIndex значение -1. Второй метод записывает в lastIndex значение -1. Вопрос товарищам знатокам: "Зачем разработчик написал два метода?"

У меня нет ответа на этот вопрос. Вполне вероятно, каждый из этих методов должен был делать что-то помимо присваивания, но в октябре у этого кода юбилей — ему будет 10 лет (коммит).

Фрагмент 16

public void removePropertyChangeListener(
  java.beans.PropertyChangeListener listener)
{
  if (listener == null)
  {
    String msg = Logging.getMessage(
      "nullValue.ListenerIsNull"
    );
    Logging.logger().severe(msg);
    throw new IllegalArgumentException(msg);
  }
this.getChangeSupport()
  .addPropertyChangeListener(listener);
}

Внимательно посмотрите на этот метод. Я серьёзно, это важно. Я даже попрошу Коди Фантастика спрятать от вас всё, что будет дальше, ибо спойлеры.

Запомнили? А теперь посмотрите на этот метод:

public void addPropertyChangeListener(
  java.beans.PropertyChangeListener listener)
{
  if (listener == null)
  {
    String msg = Logging.getMessage(
      "nullValue.ListenerIsNull"
    );
    Logging.logger().severe(msg);
    throw new IllegalArgumentException(msg);
  }
this.getChangeSupport()
  .addPropertyChangeListener(listener);
}

Теперь ответьте на вопрос: "Чем они отличаются?"

Если вы ответили "ничем", то почти правы: отличается только название. Один метод, судя по его имени, добавляет ChangeListener, а другой его убирает. При этом логика у методов идентичная. Об этом же нам сообщил анализатор.

Предупреждение PVS-Studio: V6032 It is odd that the body of method 'addPropertyChangeListener' is fully equivalent to the body of another method 'removePropertyChangeListener'. KMLRoot.java 1290

Фрагмент 17

public double getLegLength()
{
  return this.arrowLength;
}

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

public double getArrowLength()
{
  return this.arrowLength;
}

Предупреждение PVS-Studio: V6091 Suspicious getter implementation. The 'legLength' field should probably be returned instead. AttackByFirePosition.java 169

Да, разработчики забыли поменять поле объекта, которое должен возвращать этот геттер. Из-за этого вместо значения legLength возвращается arrowLength.

Комментарии излишни

Фрагмент 18

....
switch (imageType)
{
  case IMAGE_TYPE_ALPHA_RGB:
    rgbColor = 0xFF000000 + rgbColor;
    break;
//case IMAGE_TYPE_GRAY:
//  break;
//case IMAGE_TYPE_RGB:
//  break;
  case IMAGE_TYPE_GRAY_ALPHA:
    rgbColor = (rgbColor << 8) + 0xFF;
      break;
  case IMAGE_TYPE_RGB_ALPHA:
    rgbColor = (rgbColor << 8) + 0xFF;
    break;
}
....

Предупреждение PVS-Studio: V6002 The switch statement does not cover all values of the 'RPFImageType' enum: IMAGE_TYPE_RGB, IMAGE_TYPE_GRAY. NITFSImageSegment.java 281

Анализатор сообщил, что этот switch-case не покрывает всех возможных значений перечисления imageType. Это действительно так. Причём два случая, которые не покрыты, закомментированы авторами.

Да, они ничего не делают, но при этом изначально они были. В чём тут дело — загадка. Возможно, всё-таки должна была быть какая-то обработка подобных случаев.

Бесконечность — предел?

Вот этот фрагмент кода для меня, наверное, самая большая загадка проекта.

Фрагмент 19

protected String askForDatasetName(
  String suggestedName)
{
  String datasetName = suggestedName;

  for (; ; )
  {
    Object o = JOptionPane.showInputDialog(....);

    if (!(o instanceof String)) // user canceled the input
    {
      Thread.interrupted();

      String msg = Logging.getMessage(....);
      Logging.logger().info(msg);
      throw new WWRuntimeException(msg);
    }

    return WWIO.replaceIllegalFileNameCharacters((String) o);
  }
}

Предупреждение PVS-Studio: V6037 An unconditional 'return' within a loop. DataInstaller.java 273

Итак. Есть бесконечный цикл (да, for(; ; ). Это некоторый аналог while(True)), но завершается на первой же итерации. Причём никакого условия у этого return нет. Вполне вероятно, что условие здесь должно было быть.

И в целом неясно, что конкретно здесь пошло не так. Использование бесконечного цикла, по-хорошему, само по себе проблема, а здесь ещё и непонятно, зачем он вообще был сделан.

Коллекционер

Теперь посмотрим на фрагменты, в которых что-то не так с коллекциями. Например, вот такой.

Фрагмент 20

....
int[] size = new int[] {desiredSize, desiredSize};
....
if (dLon.radians > dLat.radians)
{
  size[0] = desiredSize;   <=
  size[1] = minSize; 
}
else
{
  size[0] = minSize; 
  size[1] = desiredSize;      <=
}
....

Предупреждения PVS-Studio:

V6026 This value is already assigned to the 'size[1]' variable. ExportImageOrElevations.java 415

V6026 This value is already assigned to the 'size[0]' variable. ExportImageOrElevations.java 409

Итак, здесь авторы, вероятно, забыли, что у обоих элементов в массиве значения desiredSize, и присвоили это значение ещё раз. Причём выбрали индекс элемента, исходя из условия. По-хорошему, можно было убрать одну из строчек в каждой ветке ветвления, и код работал бы ровно так же.

Фрагмент 21

public void releaseNameCopyBuffer(char[] buf)
{
  if (buf != null) {
    if (buf != _nameCopyBuffer) {       <=
      throw new IllegalArgumentException(....);
    }
  }
  ....
}

Предупреждение PVS-Studio: V6013 Arrays 'buf' and '_nameCopyBuffer' are compared by reference. Possibly an equality comparison was intended. IOContext.java 234

Здесь авторы сравнивают два массива, но делают это неправильно. Дело в том, что при их сравнении следует использовать метод Arrays.equals, потому что оператор != произведёт сравнение непосредственно ссылок на объекты, а предполагается, скорее всего, сравнение содержимого этих массивов.

Какие ваши аргументы?

А теперь посмотрим, как при передаче аргументов куда-либо что-то пошло не по плану.

Фрагмент 22

private final void _skipUtf8_3(int c)
  throws IOException, JsonParseException
{
  if (_inputPtr >= _inputEnd) {
    loadMoreGuaranteed();
  }
  //c &= 0x0F;
  c = (int) _inputBuffer[_inputPtr++];   <=
  ....
}

Предупреждение PVS-Studio: V6023 Parameter 'c' is always rewritten in method body before being used. Utf8StreamParser.java 1585

Итак, авторы принимают целочисленный аргумент c, а потом сразу же перетирают его другим значением. Зачем? У самурая нет цели, только путь...

Причём перед повторным присваиванием этого аргумента стоит строка, которая поправила бы всю ситуацию... Если бы была на месте той, на которую указывает анализатор.

Более того, подобных методов в одном и том же фрагменте несколько, они все вызываются в таком switch–case:

switch (codes[c]) {
case 1: 
  _decodeEscaped();
  break;
case 2: 
  _skipUtf8_2(c);
  break;
case 3: 
  _skipUtf8_3(c);
  break;
case 4: 
  _skipUtf8_4(c);
  break;
default:
  ....
}

Но, что самое интересное, в каждом из этих методов передаваемый параметр не используется! Например, вот метод _skipUtf8_2:

private final void _skipUtf8_2(int c)
  throws IOException, JsonParseException
{
  if (_inputPtr >= _inputEnd) {
    loadMoreGuaranteed();
  }
  c = (int) _inputBuffer[_inputPtr++];
  if ((c & 0xC0) != 0x080) {
    _reportInvalidOther(c & 0xFF, _inputPtr);
  }
}

Аргумент c точно так же перетирается, как и в методе, который был приведён выше. Но и это не предел. Вот какое сокровище спрятано в методе skipUtf8_4:

private final void _skipUtf8_4(int c)
    throws IOException, JsonParseException
{
    if (_inputPtr >= _inputEnd) {
      loadMoreGuaranteed();
    }
    int d = (int) _inputBuffer[_inputPtr++];
    if ((d & 0xC0) != 0x080) {
      _reportInvalidOther(d & 0xFF, _inputPtr);
    }
    if (_inputPtr >= _inputEnd) {
      loadMoreGuaranteed();
    }
    if ((d & 0xC0) != 0x080) {
      _reportInvalidOther(d & 0xFF, _inputPtr);
    }
    if (_inputPtr >= _inputEnd) {
      loadMoreGuaranteed();
    }
    d = (int) _inputBuffer[_inputPtr++];
    if ((d & 0xC0) != 0x080) {
      _reportInvalidOther(d & 0xFF, _inputPtr);
    }
}

Всё правильно, аргумент c здесь в принципе не используется, а вместо него вычисляется некоторая локальная переменная d.

Таким образом, ни в одном из методов, которые вызываются в switch–case, этот аргумент не нужен. Зачем его туда передавали — большой вопрос.

Не так сели...

Фрагмент 23

public static List<Layer> getLayersRemoved(
  LayerList oldList, LayerList newList)
{
  return getListDifference(newList, oldList);
}

Вроде бы ничего страшного не происходит: принимаем два параметра, потом вызываем другой метод, передавая туда эти самые параметры, но... Давайте посмотрим на сигнатуру метода, который здесь вызывается:

public static List<Layer> getListDifference(
  LayerList oldList, LayerList newList)

Да, авторы кода перепутали порядок следования аргументов. Сначала должен передаваться oldList, а после newList. Но в вызове этого метода их перепутали местам, из-за чего логика работы программы нарушится. Анализатор сказал ровно об этом же.

Предупреждение PVS-Studio: V6029 Possible incorrect order of arguments passed to method: 'newList', 'oldList'. LayerList.java 143

Итого

Вот такие ошибки нашлись в проекте WorldWind Java. В процессе чтения текста вам могло показаться, что я подшучиваю над разработчиками из NASA. Но это не так. В NASA, бесспорно, работают крутые инженеры, а я всего лишь рассказал о том, какие ошибки эти инженеры допустили :)

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

А попробовать статический анализатор кода PVS-Studio на своём проекте можно по этой ссылке.

Увидимся!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Valerii Filatov. How NASA got the planet's source code wrong.