[Kstars-devel] Suspicious Code - Need Review

[Thorsten Röder]

Hi,

Christoph Bartoschek did a great job 
finding a lots of suspicious lines of code in 3.5.2 and posted these
issues on the kde-core-devel list.

I tried to fix the mentioned problems in parts that "belongs" to kstars.
Please have a look at the attached patch, would be great if someone can
give me feedback about this, so I can commit that if its ok.

Question: How to deal with the other problems "belonging" to the 
INDI library itself? Is this library an external library maintained 
by someone not on this list? Or is the INDI library developed with kstars?


These are/were the problems Christoph Bartoschek found:

___ IN KSTARS___, should be fixed with the attached patch:

> - kstars/kstars/fitshistogram.cpp:148
> 
> If binSize is 0 but buffer is not NULL, then line 144 is not 
> executed and line 148 has a division by 0.
> 
Fixed.
> - kstars/kstars/fitsprocess.cpp:147
> 
> Is narray leaking here?  If yes, why not using std::vector?
>
Fixed.
> - kstars/kstars/indidevice.cpp:874
> - kstars/kstars/indidevice.cpp:885 (similar)
> - kstars/kstars/indidevice.cpp:898 (similar)
> 
> In line 872 you delete pp and use it here again.
> 
Fixed. PLEASE INVESTIGATE PATCH.
> - kstars/kstars/fitsviewer.cpp:308
> 
> If buffer is != NULL here, than the memory is leaking here. 
> std::vector is beffer for such tasks.
> 
Fixed.
> - kstars/kstars/indistd.cpp:492
> 
> Use delete [] here or better a std::string or QString for tempPrefix.
> 
Fixed.
> - kstars/kstars/kstarsdata.cpp:553
> 
> If ok is true but nn != 2, then you delete seg in line 546. 
> But you use it again in line 553.
> 
Fixed. PLEASE INVESTIGATE PATCH.
> ------------------------------------
> Problems involving the NULL pointer:
> ------------------------------------
> 
> - kstars/kstars/kswizard.cpp:143
> 
> If the if condition in line 134 is false, then line 143 crashes.
> 
Fixed. PLEASE INVESTIGATE PATCH.
> -----------------------------------------------------------------
> Lines where boolean expressions are used in non-boolean contexts:
> 
> I suspect that at least the lines marked with !!! are bugs
> -----------------------------------------------------------------
> 
> - kstars/kstars/kstarsdata.cpp:1663, 1675
Fixed.



___ IN INDI library___, not yet fixed:
> - kstars/kstars/indi/apmount.cpp:657
> - kstars/kstars/indi/apmount.cpp:658
>
> tmtexts and tmtp have only 1 byte allocated, but 4 bytes (32bit
> platforms) are assigned here. Why not using std::vector and 
> std::string for such tasks? Is the memory also leaking in 
> lines 667, 668?
> 
> - kstars/kstars/indi/apogee/ApnCamera.cpp:983
> 
> If no case is selected in line 967, then RegVal is uninitialized here.
> 
> - kstars/kstars/indi/apogee/ApogeeUsbLinuxForKernel.cpp:354
> 
> retval is used uninitialized.
> 
> - kstars/kstars/indi/apogee/ApogeeUsbLinuxForKernel.cpp:367
> 
> Success is not set, if the loop in line 348 is not entered.
> 
> - kstars/kstars/indi/v4ldriver.cpp:662
> 
> Use delete [] here.  Or better a std::vector<unsigned char>. 
> This way you prevent the memory leaks in lines 625 and 641.
> 
> - kstars/kstars/indi/v4lphilips.cpp:602
> 
> The index 5 is out of bounds of this array.
> 
> - kstars/kstars/indi/sbigccd.cpp:567
> - kstars/kstars/indi/sbigccd.cpp:590
> - kstars/kstars/indi/sbigccd.cpp:578
> - kstars/kstars/indi/sbigccd.cpp:559 (only fitsData)
> 
> fitsData and compressedData are leaking memory here. Why not 
> using std::vector<unsigned char>?
> 
> - kstars/kstars/indi/sbigccd.cpp:602
> - kstars/kstars/indi/apogee_ppi.cpp:679 (similar)
> 
> After this function finished. imageB.blob points to freed 
> memory. Are there memory leaks, when the function returns to 
> early in the second case?
> 
> - kstars/kstars/indi/fli/libfli-mem.c:75
> 
> Buffer overflow. In line 68 num is set to 2*allocated.total. 
> Then allocated.pointers gets a buffer for num pointers in 
> line 70. In line 75 you start at  position allocate.total and 
> overwrite num (= 2 *
> allocated.total) elements with 0. The last allocated.total 
> elements do not belong to the buffer.
> 
> - kstars/kstars/indi/fli/libfli-filter-focuser.c:467
> 
> abs looses precision here. Maybe you want to use labs.
> 
> - kstars/kstars/indi/fli_ccd.c:898
> - kstars/kstars/indi/fli_ccd.c:908
> 
> The memory pointed to by fitsData and compressedData is leaking here.
> 
>  - kstars/kstars/indi/temmadriver.c:73
>  
>  Buffer overflow.  PortT->text has place for 10 chars, but 
> strcpy copies
>  11 into it. Note that strcpy adds the \0.
> 
>  kstars/kstars/indi/celestronprotocol.c:562
>  
> The loop is executed at most once.
> 
> - kstars/kstars/indi/webcam/v4l2_base.cpp:1116
> 
> num_ctrls is leaking here.
>
TR BTW: numbers is leaking here too.
> -----------------------------------------------------------------
> Lines where boolean expressions are used in non-boolean contexts:
> 
> I suspect that at least the lines marked with !!! are bugs
> -----------------------------------------------------------------
> 
> - kstars/kstars/indi/apogee/CameraIO_LinuxPPI.cpp:251, 270, 300
> - kstars/kstars/indi/apogee/CameraIO_LinuxPCI.cpp:301, 320, 348
> - kstars/kstars/indi/apogee/CameraIO_LinuxISA.cpp:247, 266, 294

Thorsten