Is PHP compilable?! PVS-Studio searches for errors in PeachPie
Tags: dotnet, csharp, php, compiler
Published: 2021-08-17 09:24:49


PHP is widely known as an interpreted programming language used mainly for website development\. However, few people know that PHP also has a compiler to \.NET – PeachPie\. But how well is it made? Will the static analyzer be able to find actual bugs in this compiler? Let's find out\!

![0855_Peachpie_check/image1.png](https://import.viva64.com/docx/blog/0855_Peachpie_check/image1.png)

It's been a while since we posted articles on the C\# projects check using PVS\-Studio\.\.\. And we still have to make the 2021 Top list of bugs \(by the way, 2020 Top 10 bugs, you can find [here](https://pvs-studio.com/en/blog/posts/csharp/0787/)\)\! Well, we need to mend our ways\. I am excited to show you a review of the [PeachPie](https://www.peachpie.io/) check results\.

![0855_Peachpie_check/image2.png](https://import.viva64.com/docx/blog/0855_Peachpie_check/image2.png)

To begin with, let me tell you a little about the project\. PeachPie is a modern, open\-source PHP language compiler and runtime for the \.NET Framework and \.NET\. It is built on top of the Microsoft Roslyn compiler platform and is based on the first\-generation [Phalanger](https://github.com/DEVSENSE/Phalanger) project\. In July 2017, the project became a member of the [\.NET Foundation](https://dotnetfoundation.org/)\. The source code is available in the [GitHub repository](https://github.com/peachpiecompiler/peachpie)\.

By the way, our C\# analyzer also makes extensive use of the [Roslyn](https://github.com/dotnet/roslyn) capabilities, so in a way, PeachPie and PVS\-Studio have something in common :\)\. We've worked with Roslyn before\. Moreover, we wrote a whole [article](https://pvs-studio.com/en/blog/posts/csharp/0399/) about the basics of working with this platform\.

To check PeachPie, we had to install the analyzer, open the project in Visual Studio or Rider and run the analysis using the PVS\-Studio plugin\. For more details, see the [documentation](https://pvs-studio.com/en/docs/)\.

It was entertaining to check such a large and serious project\. I hope you'll also enjoy my review of the bugs found in PeachPie\. Have fun reading\!

## WriteLine problems

Well, let's start with an easy one :\) Sometimes bugs can appear in the most unexpected and at the same time the simplest places\. For example, an error may even appear in an easy *WriteLine* function call:

```cs
public static bool mail(....)
{
  // to and subject cannot contain newlines, replace with spaces
  to = (to != null) ? to.Replace("\r\n", " ").Replace('\n', ' ') : "";
  subject = (subject != null) ? subject.Replace("\r\n", " ").Replace('\n', ' ')
                              : "";

  Debug.WriteLine("MAILER",
                  "mail('{0}','{1}','{2}','{3}')",
                  to,
                  subject,
                  message, 
                  additional_headers);

  var config = ctx.Configuration.Core;
  
  ....
}
```

The [V3025](https://pvs-studio.com/en/w/v3025/) warning: Incorrect format\. A different number of format items is expected while calling 'WriteLine' function\. Arguments not used: 1st, 2nd, 3rd, 4th, 5th\. Mail\.cs 25

You would think, what has gone wrong? Everything seems to be fine\. Wait a minute, though\! What argument should pass the format?

Well, let's take a look at the *Debug\.WriteLine *declaration:

```cs
public static void WriteLine(string format, params object[] args);
```

The format string should be passed as the first argument, and the first argument in the code is *"MAILER"*\. Obviously, the developer mixed up the methods and passed the arguments incorrectly\.

## Same cases in switch

This section is devoted to warnings associated with performing the same actions in different case branches:

```cs
private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.DisallowNullAttribute":
        result |= FlowAnalysisAnnotations.DisallowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullAttribute":
        result |= FlowAnalysisAnnotations.MaybeNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute":
        if (TryGetBoolArgument(attr, out bool maybeNullWhen))
        {
          result |= maybeNullWhen ? FlowAnalysisAnnotations.MaybeNullWhenTrue
                                  : FlowAnalysisAnnotations.MaybeNullWhenFalse;
        }
        break;
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
    }
  }
}
```

This fragment contains if not an error, then at least a strange thing\. How quickly can you find it?

However, don't waste your time, the analyzer found everything for us:

```cs
private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      ....
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;              // <=
        break;
    }
  }
}
```

The [V3139](https://pvs-studio.com/en/w/v3139/) warning: Two or more case\-branches perform the same actions\. ReflectionUtils\.Nullability\.cs 170

Isn't it strange that two different cases are handled the same way? In fact, no, this happens quite often\. However, there are 2 peculiarities\.

Firstly, it's worth noting that there is more graceful way to treat two different cases in the same way\. You can rewrite the above fragment as follows:

```cs
switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
}
```

However, developers often neglect this convenient method and prefer copy\-paste\. Therefore, the presence of two identical branches doesn't seem so terrible\. The fact that the *FlowAnalysisAnnotations* enumeration has, among others, the *FlowAnalysisAnnotations\.NotNull *value is much more suspicious\. This value seems to be used when the* "System\.Diagnostics\.CodeAnalysis\.NotNullAttribute"* value is processed:

```cs
switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.NotNull;              // <=
    break;
}
```

## Immutable DateTime

Developers [often make mistakes](https://pvs-studio.com/en/blog/examples/v3010/) because they don't understand how the features of the "modifying" methods work\. Here is the bug found in PeachPie:

```cs
using System_DateTime = System.DateTime;

internal static System_DateTime MakeDateTime(....) { .... }

public static long mktime(....)
{
  var zone = PhpTimeZone.GetCurrentTimeZone(ctx);
  var local = MakeDateTime(hour, minute, second, month, day, year);

  switch (daylightSaving)
  {
    case -1:
      if (zone.IsDaylightSavingTime(local))
        local.AddHours(-1);                   // <=
      break;
    case 0:
      break;
    case 1:
      local.AddHours(-1);                     // <=
      break;
    default:
      PhpException.ArgumentValueNotSupported("daylightSaving", daylightSaving);
      break;
  }
  return DateTimeUtils.UtcToUnixTimeStamp(TimeZoneInfo.ConvertTime(local, 
                                                                   ....));
}
```

The PVS\-Studio warnings:

* [V3010](https://pvs-studio.com/en/w/v3010/) The return value of function 'AddHours' is required to be utilized\. DateTimeFunctions\.cs 1232
* [V3010](https://pvs-studio.com/en/w/v3010/) The return value of function 'AddHours' is required to be utilized\. DateTimeFunctions\.cs 1239

The analyzer reports that the results of calls should be recorded somewhere – otherwise, they do not make any sense\. The fact is, methods like *AddHours* don't change the original object – instead, a new object is returned, and it differs from the original one accordingly\. It's difficult to say how critical this mistake is, but it's clear that the code fragment doesn't work correctly\.

## Try\-methods with peculiarities

Try\-methods are often super convenient for developing apps in C\#\. The best\-known try\-methods are *int\.TryParse*, *Dictionary\.TryGetValue*, etc\. Usually, these methods return a flag that indicates the operation success\. The result is written to the out parameter\. The PeachPie developers decided to implement their try\-methods which were supposed to work the same way\. What came of it? Let's look at the following code:

```cs
internal static bool TryParseIso8601Duration(string str,
                                             out DateInfo result,
                                             out bool negative)
{
  ....
  if (pos >= length) goto InvalidFormat;

  if (s[pos++] != 'P') goto InvalidFormat;

  if (!Core.Convert.TryParseDigits(....))
    goto Error;
  
  if (pos >= length) goto InvalidFormat;

  if (s[pos] == 'Y')
  {
    ....

    if (!Core.Convert.TryParseDigits(....)) 
      goto Error;

    if (pos >= length) goto InvalidFormat;
  }
  ....
  InvalidFormat:
  Error:

    result = default;
    negative = default;
    return false;
}
```

This method is shortened for readability\. You can find the full method by clicking the [link](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Library/DateTime/DateTimeParsing.cs)\. *Core\.Convert\.TryParseDigits* is called many times in the method\. In cases when such a call returns *false*, the thread of execution jumps to the *Error *label, which is logical\.

On the *Error* label,* *default values* *are assigned* to out\-*parameters\. Then, the method returns *false*\. Everything looks logical – the *TryParseIso8601Duration *method behaves exactly like standard try\-methods\. Well\.\.\. At least, it's what it looks like\. In fact, it's not like that :\(\.

As I mentioned earlier if *Core\.Convert\.TryParseDigits *returns *false*, the code jumps to the *Error *label, where the bug/issue handling is performed\. However, here's the trouble – the analyzer reports that *TryParseDigits* never returns *false*:

The [V3022](https://pvs-studio.com/en/w/v3022/) warning: Expression '\!Core\.Convert\.TryParseDigits\(s, ref pos, false, out value, out numDigits\)' is always false\. DateTimeParsing\.cs 1440

If the negation of the call result is always *false*, then the call always returns *true*\. What a specific behavior for the try\-method\! Is the operation always successful? Let's finally look at *TryParseDigits*:

```cs
public static bool TryParseDigits(....)
{
  Debug.Assert(offset >= 0);

  int offsetStart = offset;
  result = 0;
  numDigits = 0;

  while (....)
  {
    var digit = s[offset] - '0';

    if (result > (int.MaxValue - digit) / 10)
    {
      if (!eatDigits)
      {
        // overflow
        //return false;
        throw new OverflowException();
      }

      ....

      return true;
    }

    result = result * 10 + digit;
    offset++;
  }

  numDigits = offset - offsetStart;
  return true;
}
```

The method does always return *true*\. But the operation may fail – in this case, an exception of the *OverflowException* type is thrown\. As for me, this is clearly not what you expect from a try\-method :\)\. By the way, there is a line with *return false*, but it is commented out\.

Perhaps, the use of an exception here is somehow justified\. But according to the code, it seems that something went wrong\. *TryParseDigits* and *TryParseIso8601Duration* using it are supposed to work like the usual try\-methods – return *false* in case of failure\. Instead, they throw unexpected exceptions\.

## Default argument value

The following analyzer message is simpler, but it also points to a rather strange code fragment:

```cs
private static bool Put(Context context,
                        PhpResource ftp_stream,
                        string remote_file,
                        string local_file,
                        int mode,
                        bool append,
                        int startpos)
{ .... }

public static bool ftp_put(Context context,
                           PhpResource ftp_stream,
                           string remote_file,
                           string local_file,
                           int mode = FTP_IMAGE,
                           int startpos = 0)
{
    return Put(context,
               ftp_stream,
               remote_file,
               local_file,
               mode = FTP_IMAGE, // <=
               false,
               startpos);
}
```

The [V3061](https://pvs-studio.com/en/w/v3061/) warning: Parameter 'mode' is always rewritten in method body before being used\. Ftp\.cs 306

The *ftp\_put* method accepts a number of parameters as input, one of the parameters is *mode*\. It has a default value, but when it's called, clearly, you can set another value\. However, this doesn't affect anything – *mode* is always overwritten, and the *Put* method always receives the value of the *FTP\_IMAGE* constant\.

It's difficult to say why everything is written this way – the construct seems meaningless\. An error is most likely to be here\.

## Copy\-paste sends greetings

The following code fragment looks like a copy\-paste victim:

```cs
public static PhpValue filter_var(....)
{
  ....
  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_PRIV_RANGE)
  {
    throw new NotImplementedException();
  }

  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_RES_RANGE)
  {
    throw new NotImplementedException();
  }
  ....
}
```

The [V3127](https://pvs-studio.com/en/w/v3127/) warning: Two similar code fragments were found\. Perhaps, this is a typo and 'NO\_RES\_RANGE' variable should be used instead of 'NO\_PRIV\_RANGE' Filter\.cs 771

It seems, the second condition had to be written this way:

*\(flags & \(int\)FilterFlag\.**NO\_RES\_RANGE**\) == \(int\)FilterFlag\.NO\_RES\_RANGE*

Anyway, this option looks more logical and clear\.

## Just an extra check in the if statement

Let's diversify our article with usual redundant code:

```cs
internal static NumberInfo IsNumber(....)
{
  ....
  int num = AlphaNumericToDigit(c);

  // unexpected character:
  if (num <= 15)
  {
    if (l == -1)
    {
      if (   longValue < long.MaxValue / 16 
          || (   longValue == long.MaxValue / 16 
              && num <= long.MaxValue % 16))         // <=
      {
        ....
      }
      ....
    }
    ....
  }
  ....
}
```

The [V3063](https://pvs-studio.com/en/w/v3063/) warning: A part of conditional expression is always true if it is evaluated: num <= long\.MaxValue % 16\. Conversions\.cs 994

Firstly, I'd like to say that the function code is significantly shortened for readability\. Click on the [link](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Runtime/Conversions.cs) to see the full *IsNumber *source code – but let me warn you – it's not easy to read\. The function contains more than 300 code lines\. It seems to go beyond accepted "one screen" :\)\.

Let's move on to the warning\. In the outer block the value of the *num* variable is checked – it must be less than or equal to 15\. In the inner block *num* is checked – it must be less than or equal to *long\.MaxValue % 16*\. In doing so, the value of this expression is 15 – it's easy to check\. The code turns out to check twice that *num* is less than or equal to 15\.

This warning hardly indicates a real bug – someone just wrote an extra check\. Maybe it was done on purpose – for example, to ease the reading of this exact code\. Although the use of some variable or constant to store the comparison result seems to be an easier option\. Anyway, the construct is redundant, and it's the static analyzer duty to report this\.

## Could there be null?

Developers often miss checks for *null*\. The situation is particularly interesting when a variable was checked in one place of the function, and in another \(where it can still be *null*\) – they forgot or didn't find it necessary\. And here we can only guess whether the check was redundant or there was a lack of it in some places\. *Null* checks do not always involve the use of comparison operators – for example, the code fragment below shows that the developer used the [null\-conditional operator](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/member-access-operators):

```cs
public static string get_parent_class(....)
{
  if (caller.Equals(default))
  {
    return null;
  }

  var tinfo = Type.GetTypeFromHandle(caller)?.GetPhpTypeInfo();
  return tinfo.BaseType?.Name;
}
```

The [V3105](https://pvs-studio.com/en/w/v3105/) warning: The 'tinfo' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. Objects\.cs 189

According to the developer, the *Type\.GetTypeFromHandle\(caller\)* call can return *null* – that's why "?\." was used to call *GetPhpTypeInfo*\. The [documentation](https://docs.microsoft.com/en-us/dotnet/api/system.type.gettypefromhandle?view=net-5.0) proves that it's possible\. 

Yay, "?\." saves from one exception\. If the *GetTypeFromHandle* call returns *null*, then the *tinfo* variable* *is also assigned *null*\. But when you try to access the *BaseType* property, another exception is thrown\. Most likely, the last line misses another "?":

*return tinfo**?**\.BaseType?\.Name;*

## Fatal warning and exceptions

*Get ready, in this part you will find a real investigation\.\.\.*

Here we have another warning related to *null* check\. The triggering turned out to be much more exciting than it looked at first glance\. Take a look at the code fragment:

```cs
static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context));
  }

  return context.HashAlgorithm;
}
```

The [V3125](https://pvs-studio.com/en/w/v3125/) warning: The 'context' object was used after it was verified against null\. Check lines: 3138, 3133\. Hash\.cs 3138

Yes, the variable is checked for *null*, and then the property access without any verification occurs\. However, look what happens if the variable value is *null*:

```cs
PhpException.ArgumentNull(nameof(context));
```

It appears that if the *context *is equal to *null*, the execution thread doesn't get to the *HashAlgorithm* property access\. Therefore, this code is safe\. Is it a false positive?

Of course, the analyzer can make mistakes\. However, I know that PVS\-Studio can handle such situations – the analyzer should have known that at the time of accessing *HashAlgorithm*, the *context* variable cannot be equal to *null*\.

But what exactly does the *PhpException\.ArgumentNull* call do? Let's take a look:

```cs
public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}
```

Hmm, looks like something is thrown\. Pay attention to the first argument of the call — *PhpError\.Warning*\. Hmm, well, let's move on to the *Throw* method:

```cs
public static void Throw(PhpError error, string formatString, string arg0)
{
  Throw(error, string.Format(formatString, arg0));
}
```

Basically, there's nothing interesting here, take a look at another *Throw *overloading:

```cs
public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}
```

And here is what we are looking for\! It turns out that under the hood of this entire system there is *PhpFatalErrorException*\. The exception seems to be thrown occasionally\.

Firstly, it's worth looking at the places where the handlers of the *OnError* event are registered\. They can throw exceptions too – that would be a little unexpected, but you never know\. There are a few handlers, and they all are related to logging the corresponding messages\. One handler is in the [PhpHandlerMiddleware file](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.AspNetCore.Web/PhpHandlerMiddleware.cs):

```cs
PhpException.OnError += (error, message) =>
{
  switch (error)
  {
    case PhpError.Error:
      logger.LogError(message);
      break;

    case PhpError.Warning:
      logger.LogWarning(message);
      break;

    case PhpError.Notice:
    default:
      logger.LogInformation(message);
      break;
  }
};
```

Two another handlers are in the [PhpException](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Runtime/Errors.cs) class:

```cs
// trace output
OnError += (error, message) =>
{
  Trace.WriteLine(message, $"PHP ({error})");
};

// LogEventSource
OnError += (error, message) =>
{
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    LogEventSource.Log.HandleFatal(message);
  }
  else
  {
    LogEventSource.Log.HandleWarning(message);
  }
};
```

Thus, event handlers do not generate any exceptions\. So, let's go back to the *Throw* method\.

```cs
public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}
```

As everything is clear with *OnError*, let's take a closer look at the condition:

```cs
(error & (PhpError)PhpErrorSets.Fatal) != 0
```

The *error* parameter stores the value of the *PhpError* enumeration\. Earlier, we noticed that the *error* parameter receives *PhpError\.Warning*\. An exception is thrown if the result of applying "bitwise AND" to the *error *and *PhpErrorSets\.Fatal* is non\-zero\.

The *PhpErrorSets\.Fatal *value is a "union" of the *PhpError *enumeration elements created by the "bitwise OR" operation:

```cs
Fatal =   PhpError.E_ERROR | PhpError.E_COMPILE_ERROR
        | PhpError.E_CORE_ERROR | PhpError.E_USER_ERROR
```

Below you can see the values of all the enumeration elements mentioned earlier:

```cs
E_ERROR = 1,
E_WARNING = 2,
E_CORE_ERROR = 16,
E_COMPILE_ERROR = 64,
E_USER_ERROR = 256,
Warning = E_WARNING
```

The *error & \(PhpError\)PhpErrorSets\.Fatal* operation returns a non\-zero value only if the *error* parameter has one of the following values or a combination of them:

```cs
PhpError.E_ERROR,
PhpError.E_COMPILE_ERROR,
PhpError.E_CORE_ERROR,
PhpError.E_USER_ERROR
```

If the *error* parameter contains the *PhpError\.Warning *value that equals *PhpError\.E\_WARNING*, the result of the "bitwise AND" operation is zero\. Then the condition for throwing *PhpFatalErrorException* is not met\.

Let's go back to the *PhpException\.ArgumentNull* method:

```cs
public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}
```

We found out that when the *PhpError\.Warning *value is passed, there is no exception\. Perhaps, the developer didn't want the exception to be thrown in cases when an unexpected *null* is passed\. It's just\.\.\.

```cs
static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context)); // no exceptions
  }

  return context.HashAlgorithm; // context is potential null
}
```

If *PhpException\.ArgumentNull* does not throw an exception \(which is unexpected\), then when we access the *HashAlgorithm* property, *NullReferenceException* occurs anyway\!

You might ask: should an exception be thrown or not? If it should, then it makes more sense to use the same *PhpFatalErrorException*\. If no one expects an exception here, then you need to correctly process the *null *value of the *context* parameter\. For example, you can use "?\."\. Anyway, the analyzer dealt with this situation and even helped to understand the issue\.

## Another extra check? An exception again\!

The last case proves that expecting an exception, you can get an unexpected *null*\. The fragment below shows the opposite case:

```cs
public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    return node.Value;
  else
    return PhpValue.Null;
}
```

The [V3022](https://pvs-studio.com/en/w/v3022/) warning: Expression 'node \!= null' is always true\. Datastructures\.cs 432

Well, there's no *null* here, then so be it\! Why grumble? However, usually *null* is expected in cases when something is wrong\. The code shows that this is exactly the case\. But the analyzer insists that there couldn't be *null*\.

You might think, it's all about the *Debug\.Assert *call in this case\. For better or for worse, this call doesn't affect the analyzer warnings\.

If it's not about *Debug\.Assert*, then what is it about? Why does the analyzer "thinks" that *node* is never equal to *null*? Let's take a look at the *GetNodeAtIndex* method, that returns the value written to *node*:

```cs
private LinkedListNode<PhpValue> GetNodeAtIndex(PhpValue index)
{
  return GetNodeAtIndex(GetValidIndex(index));
}
```

Well, we go deeper\. Take a look at the *GetNodeAtIndex* method called here:

```cs
private LinkedListNode<PhpValue> GetNodeAtIndex(long index)
{
  var node = _baseList.First;
  while (index-- > 0 && node != null)
  {
    node = node.Next;
  }

  return node ?? throw new OutOfRangeException();
}
```

Look\! It seems that the method could return *null*\.\.\. No such luck\! If the loop is terminated, and *node* is equal to *null*, an exception is thrown\. This way, no *null* can be returned\.

In case of an unexpected situation, the *GetNodeAtIndex* method doesn't return *null*, as expected in the *offsetGet* method code:

```cs
public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset); // potential null expected

  Debug.Assert(node != null);

  if (node != null) // always true
    return node.Value;
  else
    return PhpValue.Null; // unreachable
}
```

When a developer reviews this method, they can easily get deceived\. According to the code fragment, it seems that the correct value or *PhpValue\.Null* is returned\. In fact, this method can throw an exception\.

The unexpected behavior of only one method in the call chain leads to unexpected behavior of all these methods – such a troublemaker\! This example illustrates how useful static analysis is\. It finds such problems automatically\.

By the way, there is a similar problem in the *offsetSet *method from the same class:

```cs
public void offsetSet(PhpValue offset, PhpValue value)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    node.Value = value;
}
```

The [V3022](https://pvs-studio.com/en/w/v3022/) warning: Expression 'node \!= null' is always true\. Datastructures\.cs 444

## Assignments and reassignments

Why don't we take a little break from all these investigations and have a cup of coffee?

![0855_Peachpie_check/image3.png](https://import.viva64.com/docx/blog/0855_Peachpie_check/image3.png)

While we're drinking coffee, let's take a look at a simple warning that indicates a weird code fragment:

```cs
internal StatStruct(Mono.Unix.Native.Stat stat)
{
  st_dev = (uint)stat.st_dev;
  st_ctime = stat.st_ctime_nsec;
  st_mtime = stat.st_mtime_nsec;
  st_atime = stat.st_atime_nsec;
  st_ctime = stat.st_ctime;
  st_atime = stat.st_atime;
  //stat.st_blocks;
  //stat.st_blksize;
  st_mtime = stat.st_mtime;
  st_rdev = (uint)stat.st_rdev;
  st_gid = (short)stat.st_gid;
  st_uid = (short)stat.st_uid;
  st_nlink = (short)stat.st_nlink;
  st_mode = (FileModeFlags)stat.st_mode;
  st_ino = (ushort)stat.st_ino;
  st_size = stat.st_size;
}
```

The PVS\-Studio warnings:

* [V3008](https://pvs-studio.com/en/w/v3008/) The 'st\_ctime' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 78, 75\. StatStruct\.cs 78
* [V3008](https://pvs-studio.com/en/w/v3008/) The 'st\_atime' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 79, 77\. StatStruct\.cs 79

Looks like the developer got tangled in all these assignments and made a typo somewhere\. Due to this, *st\_ctime* and *st\_atime* fields receive the values twice – and the second value is not the same as the first one\.

It's an error, isn't it? But that's no fun\! I suggest you practice your skills and search for deeper meaning\. Then try to explain in the comments why everything is the way it is\.

Meanwhile, let's keep going :\)

## These immutable strings\.\.\.

At the very beginning of this article, when you were reading about the first warnings, we mentioned the immutability of *DateTime* structure instances\. The following warnings remind us of a similar strings feature:

```cs
public TextElement Filter(IEncodingProvider enc,
                          TextElement input,
                          bool closing)
{
  string str = input.AsText(enc.StringEncoding);

  if (pending)
  {
    if (str.Length == 0) str = "\r";
    else if (str[0] != '\n') str.Insert(0, "\r"); // <=
  }

  str = str.Replace("\r\n", "\n");
  if (str.Length != 0)
  {
    pending = str[str.Length - 1] == '\r';

    if (!closing && pending) str.Remove(str.Length - 1, 1); // <=
  }

    
  return new TextElement(str);
}
```

The PVS\-Studio warnings:

* [V3010](https://pvs-studio.com/en/w/v3010/) The return value of function 'Insert' is required to be utilized\. Filters\.cs 150
* [V3010](https://pvs-studio.com/en/w/v3010/) The return value of function 'Remove' is required to be utilized\. Filters\.cs 161

Everything is simple and clear – we wanted to modify the string, but something\.\.\. went wrong :\(\.

## or throw \!= or null

Recently, we analyzed a case when a developer expected the function* *to return *null *but got an exception instead\. Here is something similar but simpler:

```cs
public static bool stream_wrapper_register(....)
{
  // check if the scheme is already registered:
  if (   string.IsNullOrEmpty(protocol)
      || StreamWrapper.GetWrapperInternal(ctx, protocol) == null)
  {
    // TODO: Warning?
    return false;
  }

  var wrapperClass = ctx.GetDeclaredTypeOrThrow(classname, true);
  if (wrapperClass == null) // <=
  {
    return false;
  }

  ....
}
```

The [V3022](https://pvs-studio.com/en/w/v3022/) warning: Expression 'wrapperClass == null' is always false\. Streams\.cs 555

Of course, you can analyze it in detail, but\.\.\. The method's name says it all\! *GetDeclaredTypeOrThrow* sort of hints that it's going to throw an exception if something goes wrong\. Again, here's the thing – this behavior is also passed to the *stream\_wrapper\_register *method\. But the developer wanted this method to return *false*\. No such luck, here is an exception\!

In fact, we've already encountered deceptive names before\. Do you remember when the *PhpException\.ArgumentNull *method call didn't actually throw an exception? So, let's check whether *GetDeclaredTypeOrThrow* throws an exception:

```cs
PhpTypeInfo GetDeclaredTypeOrThrow(string name, bool autoload = false)
{
  return GetDeclaredType(name, autoload) ??
         throw PhpException.ClassNotFoundException(name);
}
```

Well, the PeachPie developers didn't try to trick you here – it is a real exception :\)\.

## Strange 'while true'

In some cases, developers use the *true *value as the *while* loop continuation condition\. It seems to be a normal thing to do – to exit the loop, you can use *break*,* return,* or exceptions\. Actually, the loop that has some expression \(instead of the *true *keyword\) as a condition looks far more than weird\. The value of this expression always has the *true *value:

```cs
public static int stream_copy_to_stream(...., int offset = 0)
{
  ....
  if (offset > 0)
  {
    int haveskipped = 0;

    while (haveskipped != offset)  // <=
    {
      TextElement data;

      int toskip = offset - haveskipped;
      if (toskip > from.GetNextDataLength())
      {
        data = from.ReadMaximumData();
        if (data.IsNull) break;
      }
      else
      {
        data = from.ReadData(toskip, false);
        if (data.IsNull) break; // EOF or error.
        Debug.Assert(data.Length <= toskip);
      }

      Debug.Assert(haveskipped <= offset);
    }
  }
  ....
}
```

The [V3022](https://pvs-studio.com/en/w/v3022/) warning: Expression 'haveskipped \!= offset' is always true\. Streams\.cs 769

The *haveskipped* variable is declared before the loop\. It is initialized with the 0 value\. This value remains with it\.\.\. until its death\. Sounds gloomy but it is what it is\. In fact, *haveskipped* is a constant\. The value of the *offset *parameter also remains the same during the loop performance\. And it remains the same in any place of the function \(you can check it [here](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Library/Streams/Streams.cs)\)\.

Did the developer plan to make the loop continuation condition always true? Theoretically, it's possible\. But take a closer look at the loop\. The following assignment looks strange:

```cs
int toskip = offset - haveskipped;
```

What's the point, if *haveskipped* is always equal to 0?

Something is wrong with the loop\. Either a serious mistake is made here, or all these *haveskipped *weird things are the remains of some old unaccomplished ideas\.

## data == null && throw NullReferenceException

Often, the use of incorrect operators in conditions leads to bugs\. There's a similar situation in the PHP compiler:

```cs
public string ReadStringContents(int maxLength)
{
  if (!CanRead) return null;
  var result = StringBuilderUtilities.Pool.Get();

  if (maxLength >= 0)
  {
    while (maxLength > 0 && !Eof)
    {
      string data = ReadString(maxLength);
      if (data == null && data.Length > 0) break; // EOF or error.
      maxLength -= data.Length;
      result.Append(data);
    }
  }
  ....
}
```

The [V3080](https://pvs-studio.com/en/w/v3080/) warning: Possible null dereference\. Consider inspecting 'data'\. PhpStream\.cs 1382

The value of the *data* variable is checked in the loop\. If the variable equals *null* and its *Length* property has a positive value, then the loop exit occurs\. Clearly, it's impossible\. Moreover, we have an exception when accessing the *Length *variable that has the *null* value\. Here, the access deliberately takes place when *data = null*\.

Given the developer's comment, I would rewrite the condition something like this:

```cs
data == null || data.Length == 0
```

However, it doesn't mean that this is the correct handling option – to fix this issue, it's better to do deep code analysis\.

## Wrong exception

There are also bugs that do not look so terrible but still may cause problems\. For example, in the following fragment, copy\-paste hits again:

```cs
public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}
```

The [V3013](https://pvs-studio.com/en/w/v3013/) warning: It is odd that the body of 'addGlob' function is fully equivalent to the body of 'addPattern' function \(506, line 515\)\. ZipArchive\.cs 506

The *addGlob* function clearly isn't supported, so when the function is called, there is an exception indicating that the *addGlob* function isn't supported\.

Believing me? Tricked you\! There is no exception here\. This is our old friend – *PhpException*:

```cs
public static class PhpException
{
  ....
  public static void FunctionNotSupported(string/*!*/function)
  {
    Debug.Assert(!string.IsNullOrEmpty(function));

    Throw(PhpError.Warning,
          ErrResources.notsupported_function_called,
          function);
  }
  ....
}
```

As we discussed earlier if the *Throw *method receives the *PhpError\.Warning *value, there is no exception\. But still, the appeared error is likely to be added to log or handled in some other way\.

Let's go back to the original code fragment:

```cs
public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}
```

The *addGlob* function is not supported and when it's called, the corresponding message is handled somehow – let's assume that it is added to the log\. The *addPattern *function isn't supported either, however, the corresponding message is still addressed to *addGlob*\.

Clearly, it's a copy\-paste error\. It's easy to fix – you just need to report about *addPattern*, and not about *addGlob* in the *addPattern* method:

```cs
public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addPattern));
  return false;
}
```

## Don't blame String\.Join\!

Sometimes developers forget the features of some functions\. That's why they check wrong values\. As a result, the check turns out to be meaningless, and there is no check where it has to be\. It seems that the same thing happened to the *getallheaders* function:

```cs
public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    if (headers != null)
    {
      var result = new PhpArray(16);

      foreach (var h in headers)
      {
        result[h.Key] = string.Join(", ", h.Value) ?? string.Empty;
      }

      return result;
    }
  }

  return null;
}
```

The [V3022](https://pvs-studio.com/en/w/v3022/) warning: Expression 'string\.Join\(", ", h\.Value\)' is always not null\. The operator '??' is excessive\. Web\.cs 932

It's pointless to use the "??" operator here since the *string\.Join *method never returns *null*\. But it can always throw *ArgumentNullException* \(You're welcome\!\)\.

*string\.Join *throws an exception if the passed reference to the sequence equals *null*\. Therefore, it's safer to write this line something like that:

```cs
result[h.Key] = h.Value != null ? string.Join(", ",h.Value) : string.Empty;
```

Actually, I want to know, whether it's possible for *Value *to be *null* at all? Maybe, we don't have to check anything here\. To figure it out, firstly, we need to understand where the *headers* collection came from\.

```cs
public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    ....
  }

  return null;
}
```

The *headers* value is taken from *webctx\.requestHeaders*, and the *webctx *value is taken from the *HttpPhpContext* property of the *ctx* object\. And the *HttpPhpContext *property\.\.\. Just take a look at this:

```cs
partial class Context : IEncodingProvider
{
  ....
  public virtual IHttpPhpContext? HttpPhpContext => null;
  ....
}
```

This, apparently, is something left for later\. If you look at the *getallheaders* method again, you see that it never works at all and simply returns *null*\.

Believing me again? But the property is virtual\! Therefore, to understand what the *getallheaders* method can return, you need to analyze descendants\. Personally, I decided to stop at this point – I still have to show other warnings\.

## Tiny assignment in a long method

Long and complex methods are likely to contain bugs\. Over time, it's difficult for developers to navigate in a large chunk of code, while it's always terrifying to change it\. Programmers add new code, the old one remains the same\. Somehow this incredible construct works, thankfully\. So, no surprise, there is some weirdness in such code\. For example, take a look at the *inflate\_fast* method:

```cs
internal int inflate_fast(....)
{
  ....
  int r;
  ....
  if (c > e)
  {
    // if source crosses,
    c -= e; // wrapped copy
    if (q - r > 0 && e > (q - r))
    {
      do
      {
        s.window[q++] = s.window[r++];
      }
      while (--e != 0);
    }
    else
    {
      Array.Copy(s.window, r, s.window, q, e);
      q += e; r += e; e = 0;                     // <=
    }
    r = 0;                                       // <=
  }
  ....
}
```

The [V3008](https://pvs-studio.com/en/w/v3008/) warning: The 'r' variable is assigned values twice successfully\. Perhaps this is a mistake\. Check lines: 621, 619\. InfCodes\.cs 621

To begin with, here is a [link](https://github.com/peachpiecompiler/peachpie/blob/cfbcc7cc34fb78097a53ec25b2ad78242160f22e/src/Peachpie.Library/Zlib/InfCodes.cs) to full code\. The method has more than two hundred lines of code with a bunch of nested constructs\. It seems that it would be difficult to puzzle it out\.

The warning is unambiguous – first, a new value is assigned to the *r* variable in the block, and then it is definitely overwritten with zero\. It's hard to say what exactly is wrong here\. Either the nullifying works somehow wrong, or the *r \+= e *construction is superfluous here\.

## null dereference in a boolean expression

Earlier, we discussed the case when an incorrectly constructed logical expression leads to an exception\. Here is another example of such a warning:

```cs
public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null && langVersion.Major > 7 
         || (langVersion.Major == 7 && langVersion.Minor >= 2);
}
```

The [V3080](https://pvs-studio.com/en/w/v3080/) warning: Possible null dereference\. Consider inspecting 'langVersion'\. AnalysisFacts\.cs 20

The code checks that the passed *langVersion *parameter doesn't equal *null*\. So, the developer assumed that *null* could be passed during the call\. Does the check save you from an exception?

Unfortunately, if the *langVersion* variable equals *null*, the value of the first part of the expression is *false*\. When the second part is calculated, an exception is thrown\.

Generally, to improve readability we need to additionally format the code fragments to post in an article\. This case isn't an exception – the expression considered earlier, in fact, was written as one line:

![0855_Peachpie_check/image5.png](https://import.viva64.com/docx/blog/0855_Peachpie_check/image5.png)

Given the comment, you can easily understand that either the operator precedence is mixed up here, or the bracket is misplaced\. The method is most likely to look as follows:

```cs
public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null 
         && (   langVersion.Major > 7 
             || langVersion.Major == 7 && langVersion.Minor >= 2);
}
```

## That's it\!

Actually, no\. The analyzer issued about 5 hundred warnings for the entire project, and there are many curious ones left waiting for the investigation\. Therefore, I still suggest you [try PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/?utm_source=peachpie-article&utm_medium=link_download) and see what else it may find in this or other projects\. Who knows, maybe you'll manage to find some bugs that are even more exciting than all warnings that I've sorted out here :\)\. Don't forget to mention the found warnings in the comments\. The bugs you found may get into the 2021 Top 10\!

Wish you good luck\!