[bug-sourceinstall] Re: invalid write

[Claudio Fontana]

Brian Gough wrote:
> It doesn't cause any major problem but there's a invalid write in the
> function srcinst_close_package() in srcinst.c
>
>     if (info->onclose & SRCINST_ONCLOSE_UPDATE) {
>         rv = _update_package(&_srcinst_state.packages, e);
>     }
>
>     info->onclose = SRCINST_ONCLOSE_NOP;
>
> When _update_package(&_srcinst_state.packages, e) is called and takes
> the branch with _remove_package_list() it causes e to be freed.  This
> causes a memory error for info->onclose = ... because info is actually
> set to e->info earlier in srcinst_close_package().  I'm not sure what
> the best fix is for that -- maybe just to return after
> _update_package()?

I think this should fix it. Can you check if it makes sense to you?


Index: actions.c
===================================================================
RCS file: /sources/sourceinstall/sourceinstall2/libsrcinst/actions.c,v
retrieving revision 1.33
diff -u -r1.33 actions.c
--- actions.c   22 Jun 2008 15:25:11 -0000      1.33
+++ actions.c   5 Dec 2009 23:54:54 -0000
@@ -875,9 +875,10 @@
     return packdir;
 }
 
-/* decide what to do based on current state */
-
-int _update_package(struct _package_list *l, struct _package_element *e)
+/* decide what to do based on current state.
+   Returns 1 on error, 0 on success.
+   If the package has been freed, *e is set to NULL. */
+int _update_package(struct _package_list *l, struct _package_element **e)
 {
 
     /* update package information on the disk based on information stored
@@ -886,17 +887,18 @@
     int rv;
     struct _package_info *info;
 
-    info = e->info;
+    info = (*e)->info;
 
     if (!info->source_location && !info->installed) {
        /* we have to remove the package. First from the filesystem, if it is
           persistant */
 
-       rv = _unlink_package_info(e->info);     /* from the filesystem */
-       _remove_package_list(l, e);     /* then from memory */
+       rv = _unlink_package_info(info); /* from the filesystem */
+       _remove_package_list(l, *e);    /* then from memory */
+       *e = 0;                         /* inform caller that it's freed. */
 
     } else {
-       rv = _save_package_info(e->info);
+       rv = _save_package_info(info);
     }
 
     return rv;
Index: actions.h
===================================================================
RCS file: /sources/sourceinstall/sourceinstall2/libsrcinst/actions.h,v
retrieving revision 1.7
diff -u -r1.7 actions.h
--- actions.h   8 Sep 2007 14:43:24 -0000       1.7
+++ actions.h   5 Dec 2009 23:54:54 -0000
@@ -74,8 +74,9 @@
 SRCINST_ERR _action_upgrade(struct _package_info *from,
                            struct _package_info *to);
 
-/* decide what to do based on current state */
-int _update_package(struct _package_list *l, struct _package_element *e);
+/* decide what to do based on current state.
+   Sets *e = NULL if the element is freed during the update. */
+int _update_package(struct _package_list *l, struct _package_element **e);
 
 /* detect a configure script, running autogen scripts if necessary. */
 
Index: srcinst.c
===================================================================
RCS file: /sources/sourceinstall/sourceinstall2/libsrcinst/srcinst.c,v
retrieving revision 1.24
diff -u -r1.24 srcinst.c
--- srcinst.c   8 Dec 2007 15:51:29 -0000       1.24
+++ srcinst.c   5 Dec 2009 23:54:54 -0000
@@ -655,10 +655,13 @@
     }
 
     if (info->onclose & SRCINST_ONCLOSE_UPDATE) {
-       rv = _update_package(&_srcinst_state.packages, e);
+       rv = _update_package(&_srcinst_state.packages, &e);
     }
 
-    info->onclose = SRCINST_ONCLOSE_NOP;
+    if (e) {
+       /* package was not freed by _update_package */
+       info->onclose = SRCINST_ONCLOSE_NOP;
+    }
     return rv;
 }