Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
Hello Vaclav. otrd., 2020. g. 4. febr., plkst. 19:51 — lietotājs Vaclav Petras () rakstīja: > > Hi Maris and Markus, > > On Tue, Feb 4, 2020 at 3:39 AM Maris Nartiss wrote: >> >> Hello Markus, >> as most of errors reported are legit, some of them are useless at the >> current philosophy of GRASS GIS. We are OK with small memory leaks, as >> modules are intended to be short running and then memory is reclaimed >> by the OS (this is faster than freeing it before exit). Of course, >> this would not be OK for long-running apps. > > > Most of them will be like what you describe, but are there some where memory > could be freed during the processing making space for more processing (e.g. > by another process)? Also the library should not leak as it should be > possible to write module which is long-running (without leaks and checkable > by Coverity or valgrind). As I wrote, it is more about philosophical approach as most of leaks are small (i.e. not freeing a mapset / location name after use etc.). If we adopt a new policy of no leaks, code could be changed in a slowly manner (case by case as need arises). > >> >> Unless we change our >> approach, resource leaks are just a noise. > > > One approach might be marking the places for Coverity to ignore making the > leak explicit (i.e., clearly intentional). Another approach might be some > G_optional_free() which could be G_free() in debug mode, but empty otherwise. This is interesting idea. I recently was thinking about having different build types. I haven't been doing any profiling, but getting rid of G_debug could be an option for release version. Having GRASS scripts running for more than 400 CPU days forces to be creative ;-) >> >> >> I would vote against integrating Coverty scan into CI — just to keep >> noise level down. It is useless to get regular reports if we do not >> plan (lack of manpower) to react to them. > > > I agree that the current number of errors is high, but if I understand it > correctly (and Markus can correct me here), the integration with CI is not > about having the errors visible for each PR on GitHub, but rather pushing > things automatically to Coverity from Travis, so that Markus does not have to > upload that manually. +1 if it reduces load of Markus. > > Best, > Vaclav Māris. ___ grass-dev mailing list grass-dev@lists.osgeo.org https://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
Hi Maris and Markus, On Tue, Feb 4, 2020 at 3:39 AM Maris Nartiss wrote: > Hello Markus, > as most of errors reported are legit, some of them are useless at the > current philosophy of GRASS GIS. We are OK with small memory leaks, as > modules are intended to be short running and then memory is reclaimed > by the OS (this is faster than freeing it before exit). Of course, > this would not be OK for long-running apps. Most of them will be like what you describe, but are there some where memory could be freed during the processing making space for more processing (e.g. by another process)? Also the library should not leak as it should be possible to write module which is long-running (without leaks and checkable by Coverity or valgrind). > Unless we change our > approach, resource leaks are just a noise. > One approach might be marking the places for Coverity to ignore making the leak explicit (i.e., clearly intentional). Another approach might be some G_optional_free() which could be G_free() in debug mode, but empty otherwise. > > I would vote against integrating Coverty scan into CI — just to keep > noise level down. It is useless to get regular reports if we do not > plan (lack of manpower) to react to them. > I agree that the current number of errors is high, but if I understand it correctly (and Markus can correct me here), the integration with CI is not about having the errors visible for each PR on GitHub, but rather pushing things automatically to Coverity from Travis, so that Markus does not have to upload that manually. Best, Vaclav > > Just my 0.02 cents, > Māris. > > pirmd., 2020. g. 3. febr., plkst. 21:50 — lietotājs Markus Neteler > () rakstīja: > > > > Hi devs, > > > > I have re-run the coverity scan on master, results below. > > > > There is also the option of Travis integration which might be a good > idea. > > Anyone to support this? > > > > Markus > > > > > ___ > grass-dev mailing list > grass-dev@lists.osgeo.org > https://lists.osgeo.org/mailman/listinfo/grass-dev ___ grass-dev mailing list grass-dev@lists.osgeo.org https://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
Hello Markus, as most of errors reported are legit, some of them are useless at the current philosophy of GRASS GIS. We are OK with small memory leaks, as modules are intended to be short running and then memory is reclaimed by the OS (this is faster than freeing it before exit). Of course, this would not be OK for long-running apps. Unless we change our approach, resource leaks are just a noise. I would vote against integrating Coverty scan into CI — just to keep noise level down. It is useless to get regular reports if we do not plan (lack of manpower) to react to them. Just my 0.02 cents, Māris. pirmd., 2020. g. 3. febr., plkst. 21:50 — lietotājs Markus Neteler () rakstīja: > > Hi devs, > > I have re-run the coverity scan on master, results below. > > There is also the option of Travis integration which might be a good idea. > Anyone to support this? > > Markus > > ___ grass-dev mailing list grass-dev@lists.osgeo.org https://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
Markus Neteler wrote: /vector/v.net/connect.c: 145 in connect_arcs() CID 1256093: Resource leak (RESOURCE_LEAK) Variable exclude_list going out of scope leaks the storage it points to. This is probably as simple as adding Vect_destroy_list(exclude_list) to the end of connect_arcs(). /lib/vector/Vlib/open_pg.c: 803 in read_p_node() CID 1256092: Uninitialized pointer read (UNINIT) Using uninitialized value res when calling PQgetvalue. There may be some logic which means that res is always initialised at that point, but it's far from clear. /raster/r.colors/edit_colors.c: 315 in edit_colors() CID 1256091: Uninitialized value use (UNINIT) Using uninitialized value input_maps.map_types. ... and others. This appears to be a consequence of using the new option dependency functions. The various fields of input_maps are initialised in two blocks, one of which is executed if opt.file-answer is non-NULL, the other if opt.maps-answer is non-NULL. Exactly one of these may be given, due to: G_option_exclusive(opt.maps, opt.file, NULL); G_option_required(opt.maps, opt.file, NULL); However, the scanner cannot determine that this is the case, Adding an else G_fatal_error() to the file-or-maps conditional should allow it to deduce that the values cannot actually be uninitialised (assuming that it understands what the __attribute__((noreturn)) on G_fatal_error() means). -- Glynn Clements gl...@gclements.plus.com ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
Markus Neteler wrote: I have submitted r62566 from today to Coverity Scan, results below. Calling Rast_read_colors without checking return value (as is done elsewhere 50 out of 61 times). I'd suggest fixing this by removing the redundant checks and making the needed check mostly redundant. While most modules check the return code, exactly what they check for (==-1, 0, =0) isn't consistent. Rast_read_colors() either: 1. Successfully reads a colour table and returns 1. 2. Discovers that the map lacks a colour table, generates a default rainbow colour table, issues a warning, and returns -2. 3. Discovers that the map's colour table is syntactically invalid, issues a warning, and returns -1. The first case isn't an error. The second case isn't really an error for most modules. The third case indicates an actual problem (the colour table won't have been initialised) but it would be straightforward to substitute a default colour table as per the second case. For the second and third cases, the module can check the return code if a default colour table means that its output is going to be garbage (e.g. modules which use it to translate cell values to a 0..1 intensity value). Otherwise, it can just proceed using the default colour table. -- Glynn Clements gl...@gclements.plus.com ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
On Wed, Oct 29, 2014 at 9:31 AM, Markus Neteler nete...@osgeo.org wrote:
Hi,
I have uploaded the current trunk (r62476), some improvements achieved!
Analysis Summary:
New defects found: 4
Defects eliminated: 19
Below the 4 newly introduced issues, hoping for comments/fixes.
Markus
The results are available at http://scan.coverity.com/projects/1038
-- Forwarded message --
From: scan-ad...@coverity.com
Date: Wed, Oct 29, 2014 at 2:19 PM
Subject: New Defects reported by Coverity Scan for grass
To: nete...@osgeo.org
Hi,
Please find the latest report on new defect(s) introduced to grass
found with Coverity Scan.
4 new defect(s) introduced to grass found with Coverity Scan.
19 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.
New defect(s) Reported-by: Coverity Scan
Showing 4 of 4 defect(s)
** CID 1249942: Resource leak (RESOURCE_LEAK)
/raster/r.viewshed/grass.cpp: 661 in save_grid_to_GRASS(grid_ *, char
*, int, outputMode_)()
should be fixed in r62477,8
** CID 1249941: Untrusted value as argument (TAINTED_SCALAR)
** CID 1249940: Untrusted value as argument (TAINTED_SCALAR)
** CID 1249939: Uninitialized pointer read (UNINIT)
/ps/ps.map/ps_vpoints.c: 272 in PS_vpoints_plot()
should be fixed in r62479,r62480
*** CID 1249942: Resource leak (RESOURCE_LEAK)
/raster/r.viewshed/grass.cpp: 661 in save_grid_to_GRASS(grid_ *, char
*, int, outputMode_)()
655 } /* for j */
656 Rast_put_row(outfd, outrast, type);
657 } /* for i */
658 G_percent(1, 1, 1);
659
660 Rast_close(outfd);
CID 1249942: Resource leak (RESOURCE_LEAK)
Variable outrast going out of scope leaks the storage it points
to.
661 return;
662 }
663
664
665
666
*** CID 1249941: Untrusted value as argument (TAINTED_SCALAR)
/lib/segment/open.c: 89 in Segment_open()
83 close(SEG-fd);
84 if (-1 == (SEG-fd = open(SEG-fname, 2))) {
85 unlink(SEG-fname);
86 G_warning(_(Unable to re-open segment file));
87 return -4;
88 }
CID 1249941: Untrusted value as argument (TAINTED_SCALAR)
Passing tainted variable SEG-srows to a tainted sink.
89 if (0 (ret = Segment_init(SEG, SEG-fd, nseg))) {
90 close(SEG-fd);
91 unlink(SEG-fname);
92 if (ret == -1) {
93 G_warning(_(Could not read segment file));
94 return -5;
*** CID 1249940: Untrusted value as argument (TAINTED_SCALAR)
/lib/segment/init.c: 78 in Segment_init()
72 || !read_off_t(fd, SEG-ncols)
73 || !read_int(fd, SEG-srows)
74 || !read_int(fd, SEG-scols)
75 || !read_int(fd, SEG-len))
76 return -1;
77
CID 1249940: Untrusted value as argument (TAINTED_SCALAR)
Passing tainted variable SEG-srows to a tainted sink.
78 return seg_setup(SEG);
79 }
80
81
82 static int read_int(int fd, int *n)
83 {
*** CID 1249939: Uninitialized pointer read (UNINIT)
/ps/ps.map/ps_vpoints.c: 272 in PS_vpoints_plot()
266 }
267 }
268
269 /* draw the icon */
270 if ((vector.layer[vec].epstype == 0) ||
271 (vector.layer[vec].epstype == 2 !eps_exist)) {
CID 1249939: Uninitialized pointer read (UNINIT)
Using uninitialized value Symb.
272 if (Symb != NULL) {
273 symbol_draw(sname, x, y, size, rotate,
274 vector.layer[vec].width);
275 }
276 }
277 } /* for (line) */
278
279 fprintf(PS.fp, \n);
280 return 0;
To view the defects in Coverity Scan visit,
http://scan.coverity.com/projects/1038?tab=overview
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
On Mon, Oct 27, 2014 at 9:22 AM, Markus Neteler nete...@osgeo.org wrote: On Tue, Oct 21, 2014 at 4:48 AM, Glynn Clements ... ** CID 1248540: Uninitialized scalar variable (UNINIT) /imagery/i.eb.hsebal01/main.c: 200 in main() ... This needs the correct logic for option dependencies. I have contacted Yann (original author) for this. He fixed that in r62438 (+r62439). Markus ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
On Tue, Oct 21, 2014 at 4:48 AM, Glynn Clements gl...@gclements.plus.com wrote: Markus Neteler wrote: ** CID 1248526: Result is not floating-point (UNINTENDED_INTEGER_DIVISION) /imagery/i.atcorr/aerosolmodel.cpp: 388 in ... ** CID 1248523: Result is not floating-point (UNINTENDED_INTEGER_DIVISION) /raster/r.contour/cont.c: 349 in getpoint() /raster/r.contour/cont.c: 351 in getpoint() ... ** CID 1248527: Result is not floating-point (UNINTENDED_INTEGER_DIVISION) /raster/simwe/r.sim.sediment/main.c: 335 in main() ... ** CID 1248529: Result is not floating-point (UNINTENDED_INTEGER_DIVISION) ... ** CID 1248535: Untrusted value as argument (TAINTED_SCALAR) Thanks, all suggestions above submitted r62412 (scheduled for backport after a while). ** CID 1248540: Uninitialized scalar variable (UNINIT) /imagery/i.eb.hsebal01/main.c: 200 in main() ... This needs the correct logic for option dependencies. I have contacted Yann (original author) for this. Markus ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
2014-10-20 17:51 GMT+02:00 Markus Neteler nete...@osgeo.org: ** CID 1248541: Untrusted loop bound (TAINTED_SCALAR) /lib/vector/Vlib/read_pg.c: 1073 in polygon_from_wkb() hopefully fixed in r62308. Martin -- Martin Landa * http://geo.fsv.cvut.cz/gwiki/Landa ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
On Mon, Oct 20, 2014 at 12:13 PM, Martin Landa landa.mar...@gmail.com wrote: 2014-10-20 17:51 GMT+02:00 Markus Neteler nete...@osgeo.org: ** CID 1248541: Untrusted loop bound (TAINTED_SCALAR) /lib/vector/Vlib/read_pg.c: 1073 in polygon_from_wkb() hopefully fixed in r62308. Martin num_of_rings = *nrings; I'm not sure what tainted scalar means but isn't this just hiding the issue, so that the checking tool cannot find it? -- Martin Landa * http://geo.fsv.cvut.cz/gwiki/Landa ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
2014-10-20 20:02 GMT+02:00 Vaclav Petras wenzesl...@gmail.com: num_of_rings = *nrings; I'm not sure what tainted scalar means but isn't this just hiding the issue, so that the checking tool cannot find it? AFAIU, the problem is dynamically computed value in loop condition. It could be anything, so it's tainted. Does anyone knows about better solution? Martin -- Martin Landa * http://geo.fsv.cvut.cz/gwiki/Landa ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
I was wondering what that mean, so I looked it up and found this: https://communities.coverity.com/thread/2303 -Bob -Original Message- From: grass-dev-boun...@lists.osgeo.org [mailto:grass-dev-boun...@lists.osgeo.org] On Behalf Of Martin Landa Sent: Monday, October 20, 2014 11:07 AM To: Vaclav Petras Cc: GRASS developers list Subject: Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass 2014-10-20 20:02 GMT+02:00 Vaclav Petras wenzesl...@gmail.com: num_of_rings = *nrings; I'm not sure what tainted scalar means but isn't this just hiding the issue, so that the checking tool cannot find it? AFAIU, the problem is dynamically computed value in loop condition. It could be anything, so it's tainted. Does anyone knows about better solution? Martin -- Martin Landa * http://geo.fsv.cvut.cz/gwiki/Landa ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev ___ grass-dev mailing list grass-dev@lists.osgeo.org http://lists.osgeo.org/mailman/listinfo/grass-dev
Re: [GRASS-dev] Fwd: New Defects reported by Coverity Scan for grass
Markus Neteler wrote:
** CID 1248526: Result is not floating-point (UNINTENDED_INTEGER_DIVISION)
/imagery/i.atcorr/aerosolmodel.cpp: 388 in
for(int k = 1; k = mu; k++)
{
double co_n = (2 * k + 1) / k / (k + 1);
co_n will be 1.0 if k==1 and 0.0 otherwise. I suggest:
double co_n = (2.0 * k + 1) / k / (k + 1);
to evaluate the expression using floating point.
** CID 1248523: Result is not floating-point (UNINTENDED_INTEGER_DIVISION)
/raster/r.contour/cont.c: 349 in getpoint()
/raster/r.contour/cont.c: 351 in getpoint()
double ratio;
...
ratio = 1 / 2;
This is equivalent to
ratio = 0.0;
It should probably be
ratio = 0.5;
** CID 1248527: Result is not floating-point (UNINTENDED_INTEGER_DIVISION)
/raster/simwe/r.sim.sediment/main.c: 335 in main()
timesec = timesec * 60.0;
iterout = iterout * 60.0;
if ((timesec / iterout) 100.0)
G_message(_(More than 100 files are going to be created !));
timesec and iterout are both integers.
The integer division may actually be correct, in which case the
constants 60.0 and 100.0 should be replaced by integers.
** CID 1248529: Result is not floating-point (UNINTENDED_INTEGER_DIVISION)
/raster/r.sunmask/main.c: 364 in main()
/raster/r.sunmask/main.c: 385 in main()
/raster/r.sunmask/main.c: 412 in main()
/raster/r.sunmask/main.c: 423 in main()
if (sretr / 60 = 24.0) {
Same issue, probably the same solution.
** CID 1248535: Untrusted value as argument (TAINTED_SCALAR)
** CID 1248540: Uninitialized scalar variable (UNINIT)
/imagery/i.eb.hsebal01/main.c: 200 in main()
if(input_row_wet-answer
input_col_wet-answer
input_row_dry-answer
input_col_dry-answer){
m_row_wet = atof(input_row_wet-answer);
m_col_wet = atof(input_col_wet-answer);
m_row_dry = atof(input_row_dry-answer);
m_col_dry = atof(input_col_dry-answer);
}
if ((!input_row_wet-answer || !input_col_wet-answer ||
!input_row_dry-answer || !input_col_dry-answer)
!flag2-answer) {
G_fatal_error(_(Either auto-mode either wet/dry pixels coordinates
should be provided!));
}
if (flag3-answer) {
G_message(_(Manual wet/dry pixels in image coordinates));
G_message(_(Wet Pixel= x:%f y:%f), m_col_wet, m_row_wet);
G_message(_(Dry Pixel= x:%f y:%f), m_col_dry, m_row_dry);
}
The variables are only initialised if all four options are given.
The code then checks that all four options are given *if* flag2 is not
given.
The variables are used if flag3 is given.
So, if you give flag3, don't give flag2, and don't give all of four
parameters, it will read uninitialised variables.
This needs the correct logic for option dependencies.
--
Glynn Clements gl...@gclements.plus.com
___
grass-dev mailing list
grass-dev@lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/grass-dev
