2025-01-13 Coverity Issues

[jwrober]

Describe the bug
Coverity found new issues with our code based on recent PRs

8 new defect(s) introduced to longturn/freeciv21 found with Coverity Scan.
105 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 8 of 8 defect(s)

** CID 1589444: (RESOURCE_LEAK)

/tools/civmanual.cpp: 186 in manual_command(tag_types *)()
/tools/civmanual.cpp: 635 in manual_command(tag_types *)()
/tools/civmanual.cpp: 173 in manual_command(tag_types *)()

---

*** CID 1589444: (RESOURCE_LEAK)

/tools/civmanual.cpp: 186 in manual_command(tag_types *)()
180     
181         fc_snprintf(filename, sizeof(filename), "%s%d.%s",
182                     game.server.rulesetdir, manuals + 1, tag_info->file_ext);
183     
184         if (QFile::exists(filename) || !(doc = fc_fopen(filename, "w"))) {
185           qCritical(_("Could not write manual file %s."), filename);
>>>     CID 1589444:    (RESOURCE_LEAK)
>>>     Variable "my_conn" going out of scope leaks the storage "my_conn.buffer" points to.
186           return false;
187         }
188     
189         fprintf(doc, "%s", tag_info->header);
190         fprintf(doc, "<!-- Generated by freeciv21-manual version %s -->\n\n",
191                 freeciv21_version());
/tools/civmanual.cpp: 635 in manual_command(tag_types *)()
629     
630         fprintf(doc, "%s", tag_info->tail);
631         fclose(doc);
632         qInfo(_("Manual file %s successfully written."), filename);
633       } // manuals
634     
>>>     CID 1589444:    (RESOURCE_LEAK)
>>>     Variable "my_conn" going out of scope leaks the storage "my_conn.buffer" points to.
635       return true;
636     }
637     
638     /**
639        Entry point of whole freeciv-manual program
640      */
/tools/civmanual.cpp: 173 in manual_command(tag_types *)()
167       // Reset aifill to zero
168       game.info.aifill = 0;
169     
170       if (!load_rulesets(nullptr, nullptr, false, nullptr, false, false,
171                          false)) {
172         // Failed to load correct ruleset
>>>     CID 1589444:    (RESOURCE_LEAK)
>>>     Variable "my_conn" going out of scope leaks the storage "my_conn.buffer" points to.
173         return false;
174       }
175     
176       for (int imanuals = 0; imanuals < MANUAL_COUNT; imanuals++) {
177         enum manuals manuals = static_cast<enum manuals>(imanuals);
178         int i;

** CID 1589443: Insecure data handling (INTEGER_OVERFLOW)
/utility/support.cpp: 580 in cat_snprintf(char *, unsigned long, const char *, ...)()

---

*** CID 1589443: Insecure data handling (INTEGER_OVERFLOW)

/utility/support.cpp: 580 in cat_snprintf(char *, unsigned long, const char *, ...)()
574       len = qstrlen(str);
575       fc_assert_ret_val(len < n, -1);
576     
577       va_start(ap, format);
578       ret = fc_vsnprintf(str + len, n - len, format, ap);
579       va_end(ap);
>>>     CID 1589443:  Insecure data handling  (INTEGER_OVERFLOW)
>>>     "(-1 == ret) ? 18446744073709551615UL : (ret + len)", which might have underflowed, is returned from the function.
580       return (-1 == ret ? -1 : ret + len);
581     }
582     
583     /**
584        Call gethostname() if supported, else just returns -1.
585      */

---

*** CID 1589442: Performance inefficiencies (AUTO_CAUSES_COPY)

/server/unittools.cpp: 1372 in bounce_unit(unit *, int, std::function<void (bounce_event)>, std::function<void (bounce_disband_event)>)()
1366         paths = finder.find_all(bounce_destination<false>(punit));
1367       }
1368     
1369       qDebug() << "Bouncing: found" << paths.size() << "possible paths";
1370     
1371       if (!paths.empty()) {
>>>     CID 1589442:  Performance inefficiencies  (AUTO_CAUSES_COPY)
>>>     Using the "auto" keyword without an "&" causes the copy of an object of type "__gnu_cxx::__alloc_traits<std::allocator<freeciv::path>, freeciv::path>::value_type".
1372         const auto path = paths[fc_rand(paths.size())];
1373         const auto steps = path.steps();
1374         const auto end_tile = path.steps().back().location;
1375         if (on_success) {
1376           on_success({.bunit = punit, .to_tile = end_tile});
1377         }

** CID 1589441: (RESOURCE_LEAK)

/tools/civmanual.cpp: 186 in manual_command(tag_types *)()
/tools/civmanual.cpp: 635 in manual_command(tag_types *)()
/tools/civmanual.cpp: 173 in manual_command(tag_types *)()

---

*** CID 1589441: (RESOURCE_LEAK)

/tools/civmanual.cpp: 186 in manual_command(tag_types *)()
180     
181         fc_snprintf(filename, sizeof(filename), "%s%d.%s",
182                     game.server.rulesetdir, manuals + 1, tag_info->file_ext);
183     
184         if (QFile::exists(filename) || !(doc = fc_fopen(filename, "w"))) {
185           qCritical(_("Could not write manual file %s."), filename);
>>>     CID 1589441:    (RESOURCE_LEAK)
>>>     Variable "my_conn" going out of scope leaks the storage "my_conn.send_buffer" points to.
186           return false;
187         }
188     
189         fprintf(doc, "%s", tag_info->header);
190         fprintf(doc, "<!-- Generated by freeciv21-manual version %s -->\n\n",
191                 freeciv21_version());
/tools/civmanual.cpp: 635 in manual_command(tag_types *)()
629     
630         fprintf(doc, "%s", tag_info->tail);
631         fclose(doc);
632         qInfo(_("Manual file %s successfully written."), filename);
633       } // manuals
634     
>>>     CID 1589441:    (RESOURCE_LEAK)
>>>     Variable "my_conn" going out of scope leaks the storage "my_conn.send_buffer" points to.
635       return true;
636     }
637     
638     /**
639        Entry point of whole freeciv-manual program
640      */
/tools/civmanual.cpp: 173 in manual_command(tag_types *)()
167       // Reset aifill to zero
168       game.info.aifill = 0;
169     
170       if (!load_rulesets(nullptr, nullptr, false, nullptr, false, false,
171                          false)) {
172         // Failed to load correct ruleset
>>>     CID 1589441:    (RESOURCE_LEAK)
>>>     Variable "my_conn" going out of scope leaks the storage "my_conn.send_buffer" points to.
173         return false;
174       }
175     
176       for (int imanuals = 0; imanuals < MANUAL_COUNT; imanuals++) {
177         enum manuals manuals = static_cast<enum manuals>(imanuals);
178         int i;

** CID 1589440: Resource leaks (RESOURCE_LEAK)
/server/generator/startpos.cpp: 504 in create_start_positions(map_startpos, unit_type *)()

---

*** CID 1589440: Resource leaks (RESOURCE_LEAK)

/server/generator/startpos.cpp: 504 in create_start_positions(map_startpos, unit_type *)()
498       for (k = 1; k <= wld.map.num_continents; k++) {
499         sum += islands[islands_index[k]].starters;
500         if (islands[islands_index[k]].starters != 0) {
501           qDebug("starters on isle %i", k);
502         }
503       }
>>>     CID 1589440:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "data" going out of scope leaks the storage "data.value" points to.
504       fc_assert_ret_val(player_count() <= sum, false);
505     
506       // now search for the best place and set start_positions
507       while (map_startpos_count() < player_count()) {
508         if ((ptile =
509                  rand_map_pos_filtered(&(wld.map), &data, is_valid_start_pos))) {

---

*** CID 1589439: Performance inefficiencies (COPY_INSTEAD_OF_MOVE)

/client/tileset/layer_water.cpp: 49 in freeciv::layer_water::initialize_extra(const extra_type *, const QString &, extrastyle_id)()
43           }
44     
45           data.sprites.push_back(sprites);
46         }
47         terrain_type_iterate_end;
48     
>>>     CID 1589439:  Performance inefficiencies  (COPY_INSTEAD_OF_MOVE)
>>>     "data" is copied and then passed-by-reference as parameter to STL insertion function "std::vector<freeciv::layer_water::extra_data, std::allocator<freeciv::layer_water::extra_data> >::push_back(std::vector<freeciv::layer_water::extra_data, std::allocator<freeciv::layer_water::extra_data> >::value_type const &)", when it could be moved instead.
49         m_cardinals.push_back(data);
50       } else if (style == ESTYLE_RIVER) {
51         extra_data outlet, river;
52         river.extra = extra;
53         outlet.extra = extra;
54     

** CID 1589438: Performance inefficiencies (AUTO_CAUSES_COPY)

/server/unittools.cpp: 1373 in bounce_unit(unit *, int, std::function<void (bounce_event)>, std::function<void (bounce_disband_event)>)()

---

*** CID 1589438: Performance inefficiencies (AUTO_CAUSES_COPY)

/server/unittools.cpp: 1373 in bounce_unit(unit *, int, std::function<void (bounce_event)>, std::function<void (bounce_disband_event)>)()
1367       }
1368     
1369       qDebug() << "Bouncing: found" << paths.size() << "possible paths";
1370     
1371       if (!paths.empty()) {
1372         const auto path = paths[fc_rand(paths.size())];
>>>     CID 1589438:  Performance inefficiencies  (AUTO_CAUSES_COPY)
>>>     Using the "auto" keyword without an "&" causes the copy of an object of type "std::vector<freeciv::path::step, std::allocator<freeciv::path::step> >".
1373         const auto steps = path.steps();
1374         const auto end_tile = path.steps().back().location;
1375         if (on_success) {
1376           on_success({.bunit = punit, .to_tile = end_tile});
1377         }
1378     

** CID 1589437: Resource leaks (RESOURCE_LEAK)
/client/attribute.cpp: 251 in unserialize_hash(QHash<attr_key, void *> *, const QByteArray &)()

---

*** CID 1589437: Resource leaks (RESOURCE_LEAK)

/client/attribute.cpp: 251 in unserialize_hash(QHash<attr_key, void *> *, const QByteArray &)()
245     
246         dio_output_init(&dout, pvalue, value_length + 4);
247         dio_put_uint32_raw(&dout, value_length);
248         if (!dio_get_memory_raw(&din, ADD_TO_POINTER(pvalue, 4), value_length)) {
249           qDebug("attribute.cpp unserialize_hash() "
250                  "memory dio_input_too_short");
>>>     CID 1589437:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "pvalue" going out of scope leaks the storage it points to.
251           return A_SERIAL_FAIL;
252         }
253     
254         if (hash->contains(key)) {
255           /* There are some untraceable attribute bugs caused by the CMA that
256            * can cause this to happen. I think the only safe thing to do is