[image] Add the concept of a static image

Not all images are allocated via alloc_image().  For example: embedded
images, the static images created to hold a runtime command line, and
the images used by unit tests are all static structures.

Using image_set_cmdline() (via e.g. the "imgargs" command) to set the
command-line arguments of a static image will succeed but will leak
memory, since nothing will ever free the allocated command line.
There are no code paths that can lead to calling image_set_len() on a
static image, but there is no safety check against future code paths
attempting this.

Define a flag IMAGE_STATIC to mark an image as statically allocated,
generalise free_image() to also handle freeing dynamically allocated
portions of static images (such as the command line), and expose
free_image() for use by static images.

Define a related flag IMAGE_STATIC_NAME to mark the name as statically
allocated.  Allow a statically allocated name to be replaced with a
dynamically allocated name since this is a potentially valid use case
(e.g. if "imgdecrypt --name <name>" is used on an embedded image).

Signed-off-by: Michael Brown <mcb30@ipxe.org>

diff --git a/src/arch/x86/core/runtime.c b/src/arch/x86/core/runtime.c
index 461188d04857826602d306a099daf07eaede5323..86083b1f9638a5c5ab877b96f005262cbccbcc45 100644 (file)
--- a/src/arch/x86/core/runtime.c
+++ b/src/arch/x86/core/runtime.c
@@ -70,6 +70,7 @@ static void cmdline_image_free ( struct refcnt *refcnt ) {
        struct image *image = container_of ( refcnt, struct image, refcnt );
 
        DBGC ( image, "RUNTIME freeing command line\n" );
+       free_image ( refcnt );
        free ( cmdline_copy );
 }
 
@@ -77,6 +78,7 @@ static void cmdline_image_free ( struct refcnt *refcnt ) {
 static struct image cmdline_image = {
        .refcnt = REF_INIT ( cmdline_image_free ),
        .name = "<CMDLINE>",
+       .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),
        .type = &script_image_type,
 };
 

diff --git a/src/core/image.c b/src/core/image.c
index 72885ec09f3a494bf6eae6d4615309acae9b1076..4f92dfe57bef171cc661a2a0c741059213c2d122 100644 (file)
--- a/src/core/image.c
+++ b/src/core/image.c
@@ -76,22 +76,41 @@ static int require_trusted_images_permanent = 0;
  * Free executable image
  *
  * @v refcnt           Reference counter
+ *
+ * Image consumers must call image_put() rather than calling
+ * free_image() directly.  This function is exposed for use only by
+ * static images.
  */
-static void free_image ( struct refcnt *refcnt ) {
+void free_image ( struct refcnt *refcnt ) {
        struct image *image = container_of ( refcnt, struct image, refcnt );
        struct image_tag *tag;
 
+       /* Sanity check: free_image() should not be called directly on
+        * dynamically allocated images.
+        */
+       assert ( refcnt->count < 0 );
        DBGC ( image, "IMAGE %s freed\n", image->name );
+
+       /* Clear any tag weak references */
        for_each_table_entry ( tag, IMAGE_TAGS ) {
                if ( tag->image == image )
                        tag->image = NULL;
        }
-       free ( image->name );
+
+       /* Free dynamic allocations used by both static and dynamic images */
        free ( image->cmdline );
        uri_put ( image->uri );
-       ufree ( image->data );
        image_put ( image->replacement );
-       free ( image );
+
+       /* Free image name, if dynamically allocated */
+       if ( ! ( image->flags & IMAGE_STATIC_NAME ) )
+               free ( image->name );
+
+       /* Free image data and image itself, if dynamically allocated */
+       if ( ! ( image->flags & IMAGE_STATIC ) ) {
+               ufree ( image->data );
+               free ( image );
+       }
 }
 
 /**
@@ -165,9 +184,13 @@ int image_set_name ( struct image *image, const char *name ) {
        if ( ! name_copy )
                return -ENOMEM;
 
+       /* Free existing name, if not statically allocated */
+       if ( ! ( image->flags & IMAGE_STATIC_NAME ) )
+               free ( image->name );
+
        /* Replace existing name */
-       free ( image->name );
        image->name = name_copy;
+       image->flags &= ~IMAGE_STATIC_NAME;
 
        return 0;
 }
@@ -220,6 +243,10 @@ int image_set_cmdline ( struct image *image, const char *cmdline ) {
 int image_set_len ( struct image *image, size_t len ) {
        void *new;
 
+       /* Refuse to reallocate static images */
+       if ( image->flags & IMAGE_STATIC )
+               return -ENOTTY;
+
        /* (Re)allocate image data */
        new = urealloc ( image->data, len );
        if ( ! new )
@@ -288,6 +315,13 @@ int register_image ( struct image *image ) {
        char name[8]; /* "imgXXXX" */
        int rc;
 
+       /* Sanity checks */
+       if ( image->flags & IMAGE_STATIC ) {
+               assert ( ( image->name == NULL ) ||
+                        ( image->flags & IMAGE_STATIC_NAME ) );
+               assert ( image->cmdline == NULL );
+       }
+
        /* Create image name if it doesn't already have one */
        if ( ! image->name ) {
                snprintf ( name, sizeof ( name ), "img%d", imgindex++ );

diff --git a/src/image/embedded.c b/src/image/embedded.c
index 58833ac30b4b4da934396fd2fc094214c24a85d5..028f4930873df88d129d4473f21730fe26ea089e 100644 (file)
--- a/src/image/embedded.c
+++ b/src/image/embedded.c
@@ -31,8 +31,9 @@ EMBED_ALL
 /* Image structures for all embedded images */
 #undef EMBED
 #define EMBED( _index, _path, _name ) {                                        \
-       .refcnt = REF_INIT ( ref_no_free ),                             \
+       .refcnt = REF_INIT ( free_image ),                              \
        .name = _name,                                                  \
+       .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),                  \
        .data = ( userptr_t ) ( embedded_image_ ## _index ## _data ),   \
        .len = ( size_t ) embedded_image_ ## _index ## _len,            \
 },

diff --git a/src/include/ipxe/image.h b/src/include/ipxe/image.h
index bf2d77626bef4356cb2de8155a73d235d8045f03..8ff938a17692ffb3fdbaea2ca90cb1b74296599b 100644 (file)
--- a/src/include/ipxe/image.h
+++ b/src/include/ipxe/image.h
@@ -30,14 +30,22 @@ struct image {
 
        /** URI of image */
        struct uri *uri;
-       /** Name */
+       /** Name
+        *
+        * If the @c IMAGE_STATIC_NAME flag is set, then this is a
+        * statically allocated string.
+        */
        char *name;
        /** Flags */
        unsigned int flags;
 
        /** Command line to pass to image */
        char *cmdline;
-       /** Raw file image */
+       /** Raw file image
+        *
+        * If the @c IMAGE_STATIC flag is set, then this is a
+        * statically allocated image.
+        */
        void *data;
        /** Length of raw file image */
        size_t len;
@@ -72,6 +80,12 @@ struct image {
 /** Image will be hidden from enumeration */
 #define IMAGE_HIDDEN 0x0008
 
+/** Image is statically allocated */
+#define IMAGE_STATIC 0x0010
+
+/** Image name is statically allocated */
+#define IMAGE_STATIC_NAME 0x0020
+
 /** An executable image type */
 struct image_type {
        /** Name of this image type */
@@ -185,6 +199,7 @@ static inline struct image * first_image ( void ) {
        return list_first_entry ( &images, struct image, list );
 }
 
+extern void free_image ( struct refcnt *refcnt );
 extern struct image * alloc_image ( struct uri *uri );
 extern int image_set_uri ( struct image *image, struct uri *uri );
 extern int image_set_name ( struct image *image, const char *name );

diff --git a/src/interface/efi/efi_cmdline.c b/src/interface/efi/efi_cmdline.c
index 13ad0fc35b107d5d16f24811ddc99f9c64d86b74..59bce925f8362a9b6b4fc37fb6b7b592077baf20 100644 (file)
--- a/src/interface/efi/efi_cmdline.c
+++ b/src/interface/efi/efi_cmdline.c
@@ -58,6 +58,7 @@ static void efi_cmdline_free ( struct refcnt *refcnt ) {
        struct image *image = container_of ( refcnt, struct image, refcnt );
 
        DBGC ( image, "CMDLINE freeing command line\n" );
+       free_image ( refcnt );
        free ( efi_cmdline_copy );
 }
 
@@ -65,6 +66,7 @@ static void efi_cmdline_free ( struct refcnt *refcnt ) {
 static struct image efi_cmdline_image = {
        .refcnt = REF_INIT ( efi_cmdline_free ),
        .name = "<CMDLINE>",
+       .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),
        .type = &script_image_type,
 };
 

diff --git a/src/tests/asn1_test.h b/src/tests/asn1_test.h
index c8167ed3605c43d3f76167dabdec3bae64029370..f69a4bf2a8b81f6b8fca5bb2bb19dbe7d30e248c 100644 (file)
--- a/src/tests/asn1_test.h
+++ b/src/tests/asn1_test.h
@@ -46,6 +46,7 @@ struct asn1_test {
        static struct image _name ## __image = {                        \
                .refcnt = REF_INIT ( ref_no_free ),                     \
                .name = #_name,                                         \
+               .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),          \
                .data = ( userptr_t ) ( _name ## __file ),              \
                .len = sizeof ( _name ## __file ),                      \
        };                                                              \

diff --git a/src/tests/cms_test.c b/src/tests/cms_test.c
index 5e186cdf0a9ebe145c4cc6bb476033683f8b9a59..f80fbaa86f86e14cc2bae4a992579dbcff239eda 100644 (file)
--- a/src/tests/cms_test.c
+++ b/src/tests/cms_test.c
@@ -85,6 +85,7 @@ struct cms_test_keypair {
                .image = {                                              \
                        .refcnt = REF_INIT ( ref_no_free ),             \
                        .name = #NAME,                                  \
+                       .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),  \
                        .data = ( userptr_t ) ( NAME ## _data ),        \
                        .len = sizeof ( NAME ## _data ),                \
                },                                                      \
@@ -97,6 +98,7 @@ struct cms_test_keypair {
                .image = {                                              \
                        .refcnt = REF_INIT ( ref_no_free ),             \
                        .name = #NAME,                                  \
+                       .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),  \
                        .data = ( userptr_t ) ( NAME ## _data ),        \
                        .len = sizeof ( NAME ## _data ),                \
                },                                                      \
@@ -109,6 +111,7 @@ struct cms_test_keypair {
                .image = {                                              \
                        .refcnt = REF_INIT ( ref_no_free ),             \
                        .name = #NAME,                                  \
+                       .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),  \
                        .type = &der_image_type,                        \
                        .data = ( userptr_t ) ( NAME ## _data ),        \
                        .len = sizeof ( NAME ## _data ),                \

diff --git a/src/tests/pixbuf_test.h b/src/tests/pixbuf_test.h
index d12829d89fd9ce832148b8f235d9a6061bf5a090..991c16ba31825873d0a18776615c5618d9d062a3 100644 (file)
--- a/src/tests/pixbuf_test.h
+++ b/src/tests/pixbuf_test.h
@@ -41,6 +41,7 @@ struct pixel_buffer_test {
        static struct image _name ## __image = {                        \
                .refcnt = REF_INIT ( ref_no_free ),                     \
                .name = #_name,                                         \
+               .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),          \
                .data = ( userptr_t ) ( _name ## __file ),              \
                .len = sizeof ( _name ## __file ),                      \
        };                                                              \

diff --git a/src/tests/test.c b/src/tests/test.c
index 1ec4b21ef511e0e1a07ee7a2fb0ce5a46c0cb467..9fa12e27a9ef627973c765fc32077496a59b228f 100644 (file)
--- a/src/tests/test.c
+++ b/src/tests/test.c
@@ -162,6 +162,7 @@ static struct image_type test_image_type = {
 static struct image test_image = {
        .refcnt = REF_INIT ( ref_no_free ),
        .name = "<TESTS>",
+       .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),
        .type = &test_image_type,
 };