Null check missing for input argument (source) when updating Disassembly view

[LizzMre]

Repo: https://github.com/eclipse-cdt/cdt/tree/main/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly

Bug Description

Scenario
Let’s consider that we have a Client-Server architecture where the server is sending to the client information on which lines to append to the Disassembly view from a specific file.
There might be a possibility for the server to send some incorrect data, for example: sending additional lines that cannot be found in the associated file.
In this case, the GUI will display a NullPointerException when trying to fetch the lines that don’t exist.
Even though the server might send incorrect data, it would be nice to provide the user a message with a suggestive description of the issue and prevent the GUI from crashing.

Code flow

1. Information was received and it will reach the following line:
   SourceColorerJob.runInUIThread() -> line 63

2. Next, the following method will be called:
   DisassemblyPart.updateInvalidSource() -> line 1706
   

3. From here, for each line (that must be fetched from the file) it will reach the following method: DisassemblyPart.insertSource() -> line 2756 that has multiple blocks of code similar to the sample below:

Sample from this method

4. In this method, there are some calls on SourceFileInfo.getLines() method, that will return null if it fails to get the lines from the file.

5. After the lines (source) were extracted from the file, it goes back to step 3 and it will call DisassemblyDocument.insertSource() where the source is processed, therefore it should not be null.

Bug
If a line does not exist within the file, SourceFileInfo.getLines() will catch the BadLocationException and return null (See step 4).
The source will be null and sent to DisassemblyDocument.insertSource() (See step 3).
When reaching step 5, there will be a NullPointerException for source.isEmpty() and the GUI will display the following problem:

To Reproduce

The issue has occurred in an isolating case described in the Scenario, but in order to forcingly reproduce the issue, there might be two ways:

1. Make the source null. Above line 1232 from DisassemblyDocument.insertSource(), we can add source = null;.

2. Force fetching lines that don't exist in the file. Above line 138 from SourceFileInfo.getLines(), we can add:
   int forceOffset = 10;
   first = fSource.getNumberOfLines() + forceOffset;

Expected behavior & Potential fix

1. At step 3 from Code flow, if fi.getLines() returns null, then fDocument.insertSource() should not be called, but instead the fDocument.removeSourcePosition(pos) should be called because the source position (pos) is not valid.
2. At step 5 from Code flow, a null check before line 1232 in DisassemblyDocument.insertSource() could prevent a NullPointException from happening.
3. At step 4 from Code flow, if BadLocationException is caught, it might be useful to have a Message Dialog box notification to inform the user on the issue that occurred.
   *According to the string’s externalization guidelines, this might not be approved, but it might be helpful for a user to debug the issue.

Additional information

I would be interested in creating a Pull Request to fix this issue, but I would like a confirmation on the issue described above first. Also, please address me any questions if something is not clear!

Thank you!

[jonahgraham]

Hi @LizzMre - sorry for the delay in responding. I thought I had replied, but I must have lost my draft, so here we go from scratch:

I would be interested in creating a Pull Request to fix this issue, but I would like a confirmation on the issue described above first. Also, please address me any questions if something is not clear!

I am delighted to received a PR on this issue. Your analysis of the problem looks sound.

Expected behavior & Potential fix

1. At step 3 from Code flow, if fi.getLines() returns null, then fDocument.insertSource() should not be called, but instead the fDocument.removeSourcePosition(pos) should be called because the source position (pos) is not valid.

Yes, that is a good solution.

2. At step 5 from Code flow, a null check before line 1232 in DisassemblyDocument.insertSource() could prevent a NullPointException from happening.

That also works, and this option is the minimal change to get rid of the NPE AFAICT.

3. At step 4 from Code flow, if BadLocationException is caught, it might be useful to have a Message Dialog box notification to inform the user on the issue that occurred.

I don't think there should be a message dialog here. This is because AFAIU this would be a programming error that the user cannot do anything about. Instead I would log the error to the error log. If there is some corrective action the user can take, then displaying the dialog is helpful. It is also suitable to use the dialog if the failure is such that there is no other way to indicate to the user that the displayed values are now incorrect, however I don't think this applies based on your proposal in 1.

[LizzMre]

Hi @jonahgraham

Thank you for your response! I will take into consideration your suggestions and I will create a PR on this issue.