From 9649654c3c0c20c7035764169609816db600b48a Mon Sep 17 00:00:00 2001 From: DanS Date: Wed, 15 Jul 2026 09:49:34 -0500 Subject: [PATCH] =?UTF-8?q?fix(contacts):=20adversarial-review=20fixes=20?= =?UTF-8?q?=E2=80=94=20delete=20UB,=20JPEG=20decode,=20footer=20clip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From an adversarial review of the avatar edit-dialog + image picker: - HIGH: the per-row Delete icon called doDelete() (which erases from book.entries()) INSIDE the loop iterating that same vector — out-of-bounds reads / wrong rows on the confirming click. Defer it until after the loop. - HIGH: the decode stack was compiled PNG-only (STBI_ONLY_PNG) while the image picker accepts .jpg/.jpeg/.bmp/.gif, so picking a JPEG (the common photo case) silently produced a non-loading avatar + an orphaned copy on disk. Enable JPEG/BMP/GIF decoders, and guard the pick: verify the source decodes before copying/committing (new contact_avatar_bad_image string, 8 langs). - MED: the fixed, non-scrolling edit card floored bodyH at 260*dp, which could push Save/Cancel below the card on short windows — floor lowered so the footer always stays inside. - LOW: the live-preview panel rounding is now dp-scaled (10*dp) to match the real list card it mirrors. - LOW: re-picking the same source path after its contents changed showed a stale cached texture — evict the avatar texture cache entry on re-pick. Co-Authored-By: Claude Opus 4.8 (1M context) --- res/lang/de.json | 1 + res/lang/es.json | 1 + res/lang/fr.json | 1 + res/lang/ja.json | 1 + res/lang/ko.json | 1 + res/lang/pt.json | 1 + res/lang/ru.json | 1 + res/lang/zh.json | 1 + src/ui/windows/contacts_tab.cpp | 34 ++++++++++++++++++++++++++++----- src/util/i18n.cpp | 1 + src/util/texture_loader.cpp | 5 +++++ 11 files changed, 43 insertions(+), 5 deletions(-) diff --git a/res/lang/de.json b/res/lang/de.json index 64e14ee..4719b01 100644 --- a/res/lang/de.json +++ b/res/lang/de.json @@ -288,6 +288,7 @@ "console_zoom_in": "Vergrößern", "console_zoom_out": "Verkleinern", "contact_avatar": "AVATAR", + "contact_avatar_bad_image": "Dieses Bild konnte nicht geladen werden.", "contact_avatar_badge": "Abzeichen", "contact_avatar_badge_hint": "Das Abzeichen wird automatisch anhand des Adresstyps gewählt.", "contact_avatar_choose": "Bild auswählen…", diff --git a/res/lang/es.json b/res/lang/es.json index 7f652df..2550623 100644 --- a/res/lang/es.json +++ b/res/lang/es.json @@ -288,6 +288,7 @@ "console_zoom_in": "Acercar", "console_zoom_out": "Alejar", "contact_avatar": "AVATAR", + "contact_avatar_bad_image": "No se pudo cargar esa imagen.", "contact_avatar_badge": "Insignia", "contact_avatar_badge_hint": "La insignia se elige automáticamente según el tipo de dirección.", "contact_avatar_choose": "Elegir imagen…", diff --git a/res/lang/fr.json b/res/lang/fr.json index 8380774..78b0f10 100644 --- a/res/lang/fr.json +++ b/res/lang/fr.json @@ -288,6 +288,7 @@ "console_zoom_in": "Agrandir", "console_zoom_out": "Réduire", "contact_avatar": "AVATAR", + "contact_avatar_bad_image": "Cette image n'a pas pu être chargée.", "contact_avatar_badge": "Badge", "contact_avatar_badge_hint": "Le badge est choisi automatiquement selon le type d'adresse.", "contact_avatar_choose": "Choisir une image…", diff --git a/res/lang/ja.json b/res/lang/ja.json index 14bc2a3..b015d26 100644 --- a/res/lang/ja.json +++ b/res/lang/ja.json @@ -288,6 +288,7 @@ "console_zoom_in": "拡大", "console_zoom_out": "縮小", "contact_avatar": "アバター", + "contact_avatar_bad_image": "その画像を読み込めませんでした。", "contact_avatar_badge": "バッジ", "contact_avatar_badge_hint": "バッジはアドレスの種類に応じて自動的に選ばれます。", "contact_avatar_choose": "画像を選択…", diff --git a/res/lang/ko.json b/res/lang/ko.json index 1fa3e3a..c78aeab 100644 --- a/res/lang/ko.json +++ b/res/lang/ko.json @@ -288,6 +288,7 @@ "console_zoom_in": "확대", "console_zoom_out": "축소", "contact_avatar": "아바타", + "contact_avatar_bad_image": "그 이미지를 불러올 수 없습니다.", "contact_avatar_badge": "배지", "contact_avatar_badge_hint": "배지는 주소 유형에 따라 자동으로 선택됩니다.", "contact_avatar_choose": "이미지 선택…", diff --git a/res/lang/pt.json b/res/lang/pt.json index d7e7ca5..7b8864a 100644 --- a/res/lang/pt.json +++ b/res/lang/pt.json @@ -288,6 +288,7 @@ "console_zoom_in": "Aumentar zoom", "console_zoom_out": "Diminuir zoom", "contact_avatar": "AVATAR", + "contact_avatar_bad_image": "Não foi possível carregar essa imagem.", "contact_avatar_badge": "Selo", "contact_avatar_badge_hint": "O selo é escolhido automaticamente conforme o tipo de endereço.", "contact_avatar_choose": "Escolher imagem…", diff --git a/res/lang/ru.json b/res/lang/ru.json index a0b412e..1d6ff49 100644 --- a/res/lang/ru.json +++ b/res/lang/ru.json @@ -288,6 +288,7 @@ "console_zoom_in": "Увеличить", "console_zoom_out": "Уменьшить", "contact_avatar": "АВАТАР", + "contact_avatar_bad_image": "Не удалось загрузить это изображение.", "contact_avatar_badge": "Значок", "contact_avatar_badge_hint": "Значок выбирается автоматически по типу адреса.", "contact_avatar_choose": "Выбрать изображение…", diff --git a/res/lang/zh.json b/res/lang/zh.json index d391e82..87d39d6 100644 --- a/res/lang/zh.json +++ b/res/lang/zh.json @@ -288,6 +288,7 @@ "console_zoom_in": "放大", "console_zoom_out": "缩小", "contact_avatar": "头像", + "contact_avatar_bad_image": "无法加载该图片。", "contact_avatar_badge": "标记", "contact_avatar_badge_hint": "标记会根据地址类型自动选择。", "contact_avatar_choose": "选择图片…", diff --git a/src/ui/windows/contacts_tab.cpp b/src/ui/windows/contacts_tab.cpp index 3e876bf..ec4bdbf 100644 --- a/src/ui/windows/contacts_tab.cpp +++ b/src/ui/windows/contacts_tab.cpp @@ -78,6 +78,14 @@ static const AvatarTex* getAvatarTexture(const std::string& path) { auto& slot = (s_avatarTexCache[path] = at); return slot.tex ? &slot : nullptr; } +// Drop a cached avatar texture so the next getAvatarTexture reloads from disk — needed when the file +// at that path is overwritten (re-picking the same source path whose contents changed). +static void invalidateAvatarTexture(const std::string& path) { + auto it = s_avatarTexCache.find(path); + if (it == s_avatarTexCache.end()) return; + if (it->second.tex) util::DestroyTexture(it->second.tex); + s_avatarTexCache.erase(it); +} // Draw a contact's avatar into the circle at `c` (radius `r`): a custom image (circular-cropped), a // Material icon in a tinted circle, or — the default — the Z/T type badge. @@ -281,7 +289,7 @@ void RenderContactsTab(App* app) const float pw = ImGui::GetContentRegionAvail().x; ImVec2 pMin = ImGui::GetCursorScreenPos(); ImVec2 pMax(pMin.x + pw, pMin.y + ph); - material::GlassPanelSpec g; g.rounding = 10.0f; g.fillAlpha = 22; g.borderAlpha = 40; + material::GlassPanelSpec g; g.rounding = 10.0f * dp; g.fillAlpha = 22; g.borderAlpha = 40; material::DrawGlassPanel(pdl, pMin, pMax, g); data::AddressBookEntry pv; @@ -312,7 +320,9 @@ void RenderContactsTab(App* app) // Body flexes to fill the tall card, leaving just the footer button row — so the icon grid // and notes field grow into the vertical space instead of leaving it empty. const float footerReserve = 44.0f * dp + Layout::spacingMd(); - const float bodyH = std::max(260.0f * dp, ImGui::GetContentRegionAvail().y - footerReserve); + // Never floor above what's available — the footer (Save/Cancel) must stay inside the fixed, + // non-scrolling card even on short windows. A tiny floor only guards a degenerate size. + const float bodyH = std::max(48.0f * dp, ImGui::GetContentRegionAvail().y - footerReserve); const float colGap = Layout::spacingLg(); const float bodyAvail = ImGui::GetContentRegionAvail().x; const float colW = (bodyAvail - colGap) * 0.5f; @@ -568,9 +578,17 @@ void RenderContactsTab(App* app) ImGui::SetCursorPosX(ImGui::GetCursorPosX() + std::max(0.0f, (a.x - rowW) * 0.5f)); if (material::TactileButton(TR("contact_avatar_choose"), ImVec2(chooseW, 0), btnFont)) { ImagePicker::open("", [](const std::string& src) { + // Verify the source actually decodes before committing (guards a corrupt or + // unsupported file from becoming a broken avatar + an orphaned copy on disk). + int iw = 0, ih = 0; + unsigned char* px = util::LoadRawPixelsFromFile(src.c_str(), &iw, &ih); + if (!px) { Notifications::instance().error(TR("contact_avatar_bad_image")); return; } + util::FreeRawPixels(px); std::string dst = copyAvatarImage(src); - if (!dst.empty()) { s_edit_avatar = "img:" + dst; s_edit_avatar_mode = 2; } - else Notifications::instance().error(TR("contact_avatar_copy_failed")); + if (!dst.empty()) { + invalidateAvatarTexture(dst); // in case this path was cached before an overwrite + s_edit_avatar = "img:" + dst; s_edit_avatar_mode = 2; + } else Notifications::instance().error(TR("contact_avatar_copy_failed")); }); } if (hasImg) { @@ -866,6 +884,7 @@ void RenderContactsTab(App* app) const float avR = 15.0f * dp; const float rowH = avR * 2.0f + (asCard ? 16.0f * dp : 12.0f * dp); const float round = 10.0f * dp; + bool rowDeleteRequested = false; // per-row delete is deferred until after the loop for (size_t vi = 0; vi < visibleRows.size(); ++vi) { size_t i = visibleRows[vi]; const auto& entry = book.entries()[i]; @@ -950,7 +969,9 @@ void RenderContactsTab(App* app) s_selected_index = static_cast(i); if (a == 0) doCopy(); else if (a == 1) openEdit(); - else doDelete(); + // Delete mutates book.entries() (removeEntry -> erase) — deferring it past the + // row loop avoids iterating a vector we shrink mid-loop (OOB on later rows). + else rowDeleteRequested = true; } const bool armedDel = (a == 2 && s_confirm_delete_idx == static_cast(i)); const ImU32 acol = armedDel ? material::ReadableError() @@ -967,6 +988,9 @@ void RenderContactsTab(App* app) ImGui::SetCursorScreenPos(afterRow); // undo the action buttons' cursor moves ImGui::PopID(); } + // Run the deferred per-row delete now that the loop over book.entries() has finished + // (doDelete's two-stage confirm arms on the first click and removes on the second). + if (rowDeleteRequested) doDelete(); // The per-row SetCursorScreenPos leaves the cursor at the last row's bottom with no item // submitted there; commit it so ImGui sizes the scroll child (avoids the extend-boundary warning). ImGui::Dummy(ImVec2(0.0f, 0.0f)); diff --git a/src/util/i18n.cpp b/src/util/i18n.cpp index 0629d19..be24c3d 100644 --- a/src/util/i18n.cpp +++ b/src/util/i18n.cpp @@ -1243,6 +1243,7 @@ void I18n::loadBuiltinEnglish() strings_["contact_avatar_remove"] = "Remove"; strings_["contact_avatar_image_hint"] = "The image is copied into the app so it stays available if the original moves."; strings_["contact_avatar_copy_failed"] = "Could not copy that image."; + strings_["contact_avatar_bad_image"] = "That image couldn't be loaded."; strings_["contact_global_badge_tt"] = "Global contact — visible in every wallet"; strings_["address_book_added"] = "Address added to book"; strings_["address_book_count"] = "%zu addresses saved"; diff --git a/src/util/texture_loader.cpp b/src/util/texture_loader.cpp index 59045c1..ddf4be5 100644 --- a/src/util/texture_loader.cpp +++ b/src/util/texture_loader.cpp @@ -7,7 +7,12 @@ // stb_image — single-file image loader (public domain) // Only compiled here; all other files just include the header. #define STB_IMAGE_IMPLEMENTATION +// Formats we actually decode: PNG for app assets, plus JPEG/BMP/GIF so user-chosen contact avatars +// (which are commonly JPEGs) load — must stay in sync with ImagePicker's accepted extensions. #define STBI_ONLY_PNG +#define STBI_ONLY_JPEG +#define STBI_ONLY_BMP +#define STBI_ONLY_GIF #define STBI_NO_STDIO // we do our own fread for portability #include "stb_image.h"