Hi Kojima-san Going through the patch one last time before I merge it I noticed some memory leaks
On Fri, 10 Nov 2023 at 06:27, Masahisa Kojima <masahisa.koj...@linaro.org> wrote: > > This supports to boot from the URI device path. > When user selects the URI device path, bootmgr downloads > the file using wget into the address specified by loadaddr > env variable. > If the file is .iso or .img file, mount the image with blkmap > then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI). > Since boot option indicating the default file is automatically > created when new disk is detected, system can boot by selecting > the automatically created blkmap boot option. > If the file is PE-COFF file, load and start the downloaded file. > > The buffer used to download the ISO image file must be > reserved to avoid the unintended access to the image and > expose the ramdisk to the OS. > For PE-COFF file case, this memory reservation is done > in LoadImage Boot Service. > > Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org> > Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org> > --- [...] > + > +/** > + * efi_bootmgr_image_return_notify() - return to efibootmgr callback > + * > + * @event: the event for which this notification function is registered > + * @context: event context > + */ > +static void EFIAPI efi_bootmgr_image_return_notify(struct efi_event *event, > + void *context) > +{ > + efi_status_t ret; > + > + EFI_ENTRY("%p, %p", event, context); > + ret = efi_bootmgr_release_uridp_resource(context); > + EFI_EXIT(ret); > +} > + > +/** > + * try_load_from_uri_path() - Handle the URI device path > + * > + * @uridp: uri device path > + * @lo_label: label of load option > + * @handle: pointer to handle for newly installed image > + * Return: status code > + */ > +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp, > + u16 *lo_label, > + efi_handle_t *handle) > +{ > + char *s; > + int err; > + int uri_len; > + efi_status_t ret; > + void *source_buffer; > + efi_uintn_t source_size; > + struct uridp_context *ctx; > + struct udevice *blk = NULL; > + struct efi_event *event = NULL; > + efi_handle_t mem_handle = NULL; > + struct efi_device_path *loaded_dp; > + static ulong image_size, image_addr; > + > + ctx = calloc(1, sizeof(struct uridp_context)); > + if (!ctx) > + return EFI_OUT_OF_RESOURCES; ctx is allocated here > + > + s = env_get("loadaddr"); > + if (!s) { > + log_err("Error: loadaddr is not set\n"); > + return EFI_INVALID_PARAMETER; Should be freed here > + } > + image_addr = hextoul(s, NULL); > + err = wget_with_dns(image_addr, uridp->uri); > + if (err < 0) > + return EFI_INVALID_PARAMETER; and here > + image_size = env_get_hex("filesize", 0); > + if (!image_size) > + return EFI_INVALID_PARAMETER; and here > + > + /* > + * If the file extension is ".iso" or ".img", mount it and try to load > + * the default file. > + * If the file is PE-COFF image, load the downloaded file. > + */ > + uri_len = strlen(uridp->uri); > + if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) || > + !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) { > + ret = prepare_loaded_image(lo_label, image_addr, image_size, > + &loaded_dp, &blk); > + if (ret != EFI_SUCCESS) > + goto err; > + > + source_buffer = NULL; > + source_size = 0; > + } else if (efi_check_pe((void *)image_addr, image_size, NULL) == > EFI_SUCCESS) { > + /* > + * loaded_dp must exist until efi application returns, > + * will be freed in return_to_efibootmgr event callback. > + */ > + loaded_dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, > + (uintptr_t)image_addr, > image_size); > + ret = efi_install_multiple_protocol_interfaces( > + &mem_handle, &efi_guid_device_path, loaded_dp, NULL); > + if (ret != EFI_SUCCESS) > + goto err; > + > + source_buffer = (void *)image_addr; > + source_size = image_size; > + } else { > + log_err("Error: file type is not supported\n"); > + return EFI_UNSUPPORTED; and here [...] Should we just goto err and set the return value instead of returning immediately? If yes I can fix that during the merge Thanks /Ilias