[Freeciv-Dev] Re: (PR#6907) aidiplomat: Must check the result of pf_get_path

["Gregory Berkolaiko" <Gregory.Berkolaiko@xxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=6907 >

On Fri, 21 Nov 2003, Raimar Falke wrote:

> <URL: http://rt.freeciv.org/Ticket/Display.html?id=6907 >
> 
> On Fri, Nov 21, 2003 at 03:22:27AM -0800, Gregory Berkolaiko wrote:
> > 
> > <URL: http://rt.freeciv.org/Ticket/Display.html?id=6907 >
> > 
> > On Wed, 19 Nov 2003, Raimar Falke wrote:
> > 
> > > <URL: http://rt.freeciv.org/Ticket/Display.html?id=6907 >
> > > 
> > > The function can return NULL.
> > 
> > I don't see the point of this patch/assert.  If path is NULL, there will
> > be a crash in ai_unit_execute_path, but how is it different from
> > triggering assert?
> 
> You show in the code that you understand that the function can return
> NULL. If this comment isn't there and someone will copy the code
> ... it may not break in such obvious manner as it does now.
> 
> A comment like 
> 
>   /* 
>    * ai_unit_execute_path may return NULL but this isn't a problem
>    * since it will crash in ai_unit_execute_path soon.
>    */
> 
> would also be ok. The assert is just shorter.
> 
> The best solution however is if you would just react on the NULL value
> in a real way (anything except crash).

Well, then the attached patch is a better one.  It also has the bonus of 
freeing the path that is being created.

G.

? pf_get_path2.diff
Index: ai/aidiplomat.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aidiplomat.c,v
retrieving revision 1.29
diff -u -r1.29 aidiplomat.c
--- ai/aidiplomat.c     2003/11/19 11:29:59     1.29
+++ ai/aidiplomat.c     2003/11/21 18:50:27
@@ -513,9 +513,12 @@
 
       MAPSTEP(x, y, pos.x, pos.y, DIR_REVERSE(pos.dir_to_here));
       path = pf_get_path(map, pos.x, pos.y);
-      if (!ai_unit_execute_path(punit, path) || punit->moves_left <= 0) {
+      if (!path || !ai_unit_execute_path(punit, path) 
+          || punit->moves_left <= 0) {
+        pf_destroy_path(path);
         return FALSE;
       }
+      pf_destroy_path(path);
     }
 
     if (diplomat_can_do_action(punit, DIPLOMAT_BRIBE, pos.x, pos.y)) {
@@ -667,7 +670,7 @@
     struct pf_path *path;
 
     path = pf_get_path(map, goto_dest_x(punit), goto_dest_y(punit));
-    if (ai_unit_execute_path(punit, path) && punit->moves_left > 0) {
+    if (path && ai_unit_execute_path(punit, path) && punit->moves_left > 0) {
       /* Check if we can do something with our destination now. */
       if (punit->ai.ai_role == AIUNIT_ATTACK) {
         int dist  = real_map_distance(punit->x, punit->y,
Index: common/aicore/path_finding.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/aicore/path_finding.c,v
retrieving revision 1.16
diff -u -r1.16 path_finding.c
--- common/aicore/path_finding.c        2003/11/13 19:41:09     1.16
+++ common/aicore/path_finding.c        2003/11/21 18:50:27
@@ -675,8 +675,10 @@
 ************************************************************************/
 void pf_destroy_path(struct pf_path *path)
 {
-  free(path->positions);
-  free(path);
+  if (path) {
+    free(path->positions);
+    free(path);
+  }
 }