Universal-G-Code-Sender Some potential NPE bugs

Hi all,

Our tool has found several potential NPE bugs

The first one is in runSimulation.

Variable parserThread is initialized to null and will be assigned by parserThread = new Thread(simOutputBuffer);. However, if the three instructions before the assignment raise an IOException, then the program will skip the assignment and execute if (parserThread.isAlive()). Since variable parserThread is still null, then the program will raise a NullPointerException.

The second one is caused by the return null in method center().

For examples, In getLinesFromParser, it is a NPE bug if ps.center() return null and used in center.x

If lastSegment.center() returns null and used at GcodePreprocessorUtils.generatePointsAlongArcBDring, GcodePreprocessorUtils.getAngle and finally dereferenced at end.y - start.y;

The third one is caused by the return null in method getControllerFor().

For example, one call chain is this.controller = FirmwareUtils.getControllerFor(firmware);, applySettingsToController(settings, this.controller); and controller.getCommandCreator()...

Thanks.

Nov 12 '19 07:11 JulyChen728

Nice, thanks.

Is there a way to run the tool on the master branch? The version 1.0.9 is quite old and some of the code that you are referencing has been removed.

Nov 12 '19 13:11 breiler

Nice, thanks.

Is there a way to run the tool on the master branch? The version 1.0.9 is quite old and some of the code that you are referencing has been removed.

Sure, here is the result on the master branch.

Null checks are inconsistent at lines 295 and 306, 100 and 101 (region is used in the method initialize).

In method updateGLColorArray, the null relations are also inconsistent. There is only one null check for lineColorData at line 387, but the variable is used at lines 381, 390, 393.

About methods that may return null, getStateFromStatusString returns null at 460, then the return value at line 340 is used in 341, finally, the null value may be dereferenced at line 473

Also, addLinearPointSegment may return null at line 289, the method is called at 279 and the null value may be used at 280.

lookupCode may return null at line 224, then the method is called at 637 and the null may be used at 638.

extractMotion may return null at lines 629 and 646, then the method can be called at line 81 and the null value may be used at line 82.

fromString may return null at line 92, then it is called at line 115 and its return value is used at 116.

getSettingsTextField may return null at 387. The method is called and its return value is used at line 423.

lookup may return null at 50. The method is called and its return value is used from lines 90 to 95.

An interesting one is at method SendStatusPanel() at line 74, it calls this(null); and variable backend is set to null at line 78. Note that the variable backend is frequently used in the class. Even though SendStatusPanel() seems never been called, it might not be a good idea to keep this constructor.

Thanks.

Nov 15 '19 08:11 JulyChen728

Excellent, my weekend is saved! 😁 Thanks a bunch!

Nov 15 '19 08:11 breiler

We also found a kind of potential ArrayIndexOutOfBounds bug.

The variable lookups is updated by lookups.put(record.get(0), list.toArray(new String[0]));. However, the content in list is from a CSVRecord without a file integrity check. If someone use a csv file with only two columns, then the array access at line 142, 130 or 148 will cause ArrayIndexOutOfBounds exception.

Thanks.

Nov 18 '19 11:11 JulyChen728

Hi, we have reported the bugs a while ago would you please take a look and confirm if they are real bugs. We have been conducting an experiment to measure the accuracy of our static checker. We would be deeply appreciated if you can provide some feedback!

Nov 21 '19 09:11 ITWOI

I haven't had the time to look in detail. Some of these are quite obvious a risk and should probably throw an exception or return optionals instead of null. I might get some time this weekend.

Nov 21 '19 09:11 breiler

Thanks.

Nov 21 '19 10:11 ITWOI

Sorry for taking so long getting back to you guys! I've been looking through the code and verified problems on most of the places you pointed out.

What tool are you working on?

Null checks are inconsistent at lines 295 and 306

[x] The null check is inconsistent but should never be a problem as the list is always created in the default constructor. We do however allow a null list to be assigned which should probably be asserted and throw an illegal argument exception.

Null checks are inconsistent at lines 100 and 101 (region is used in the method initialize).

[x] Yep, this one was not good. =)

In method updateGLColorArray, the null relations are also inconsistent. There is only one null check for lineColorData at line 387, but the variable is used at lines 381, 390, 393.

[ ] I had a hard time following all the code in this. We should probably look further into this

getStateFromStatusString returns null at 460, then the return value at line 340 is used in 341, finally, the null value may be dereferenced at line 473

[ ] This should be handled differently even though I think it's not likely it would happen. The solution here is to return a "unknown" status string instead of null.

addLinearPointSegment may return null at line 289, the method is called at 279 and the null value may be used at 280.

[ ] This might very well lead to NPE:s and should be fixed!

lookupCode may return null at line 224, then the method is called at 637 and the null may be used at 638.

[ ] Yep, this should probably not return null. It didn't look like the code using this methods would pass any unwanted codes to it. But to reduce the risk of introducing bugs on new code it should probably handle this differently.

extractMotion may return null at lines 629 and 646, then the method can be called at line 81 and the null value may be used at line 82.

[ ] Yes this seems like a risk.

fromString may return null at line 92, then it is called at line 115 and its return value is used at 116.

[ ] This is very unlikely it will ever happen, but should of course be fixed.

getSettingsTextField may return null at 387. The method is called and its return value is used at line 423.

[x] This would very likely cause problems when we add additional axises. Thanks!

lookup may return null at 50. The method is called and its return value is used from lines 90 to 95.

[ ] This looks like a low risk and is likely never to happen, but should of course be fixed.

An interesting one is at method SendStatusPanel() at line 74, it calls this(null); and variable backend is set to null at line 78. Note that the variable backend is frequently used in the class. Even though SendStatusPanel() seems never been called, it might not be a good idea to keep this constructor.

[x] Yes, that shouldn't work... =)

Jan 08 '20 06:01 breiler