From 6b8c94a6e3094ab75fcfe319fa2bc100f4e329ec Mon Sep 17 00:00:00 2001 From: Aidan MacDonald Date: Mon, 2 May 2022 15:23:37 +0100 Subject: Fix some non-portable alignment values UBSan reports an avalanche of unaligned pointer bugs stemming from hardcoded 4-byte alignments used in certain places. Use sizeof(long) instead to align to the machine word size. Change-Id: I28e505212462c5268afa24e95df3a103ac3e2213 --- apps/gui/skin_engine/skin_parser.c | 5 +++-- apps/plugins/pictureflow/pictureflow.c | 34 ++++++++++------------------------ apps/recorder/jpeg_load.c | 4 ++-- apps/tagcache.c | 9 ++------- lib/skin_parser/skin_buffer.c | 4 ++-- 5 files changed, 19 insertions(+), 37 deletions(-) diff --git a/apps/gui/skin_engine/skin_parser.c b/apps/gui/skin_engine/skin_parser.c index ee32c06ace..1a6861ff16 100644 --- a/apps/gui/skin_engine/skin_parser.c +++ b/apps/gui/skin_engine/skin_parser.c @@ -2538,8 +2538,9 @@ bool skin_data_load(enum screen_type screen, struct wps_data *wps_data, skin_buffer = wps_buffer; wps_buffer = (char*)buf; } - skin_buffer = ALIGN_UP(skin_buffer, 4); /* align on 4-byte boundary */ - buffersize -= 3; + + /* align to long */ + ALIGN_BUFFER(skin_buffer, buffersize, sizeof(long)); #ifdef HAVE_BACKDROP_IMAGE backdrop_filename = "-"; wps_data->backdrop_id = -1; diff --git a/apps/plugins/pictureflow/pictureflow.c b/apps/plugins/pictureflow/pictureflow.c index 5136fc56e9..83d4bb5a59 100644 --- a/apps/plugins/pictureflow/pictureflow.c +++ b/apps/plugins/pictureflow/pictureflow.c @@ -1303,9 +1303,8 @@ static int build_artist_index(struct tagcache_search *tcs, if (res < SUCCESS) return res; - ALIGN_BUFFER(*buf, *bufsz, 4); - /* finalize the artist index */ + ALIGN_BUFFER(*buf, *bufsz, alignof(struct artist_data)); tmp_artist = (struct artist_data*)*buf; for (i = pf_idx.artist_ct - 1; i >= 0; i--) tmp_artist[i] = pf_idx.artist_index[-i]; @@ -1313,7 +1312,6 @@ static int build_artist_index(struct tagcache_search *tcs, pf_idx.artist_index = tmp_artist; /* move buf ptr to end of artist_index */ *buf = pf_idx.artist_index + pf_idx.artist_ct; - ALIGN_BUFFER(*buf, *bufsz, 4); if (res == SUCCESS) { @@ -1384,14 +1382,14 @@ static int create_album_index(void) int i, j, last, final, retry, res; draw_splashscreen(buf, buf_size); - ALIGN_BUFFER(buf, buf_size, 4); + ALIGN_BUFFER(buf, buf_size, sizeof(long)); -/* Artists */ + /* Artists */ res = build_artist_index(&tcs, &buf, &buf_size); if (res < SUCCESS) return res; -/* Albums */ + /* Albums */ pf_idx.album_ct = 0; pf_idx.album_len =0; pf_idx.album_untagged_idx = 0; @@ -1407,7 +1405,6 @@ static int create_album_index(void) rb->tagcache_search_finish(&tcs); if (res < SUCCESS) return res; - ALIGN_BUFFER(buf, buf_size, 4); /* Build artist list for untagged albums */ res = create_album_untagged(&tcs, &buf, &buf_size); @@ -1415,9 +1412,8 @@ static int create_album_index(void) if (res < SUCCESS) return res; - ALIGN_BUFFER(buf, buf_size, 4); - /* finalize the album index */ + ALIGN_BUFFER(buf, buf_size, alignof(struct album_data)); tmp_album = (struct album_data*)buf; for (i = pf_idx.album_ct - 1; i >= 0; i--) tmp_album[i] = pf_idx.album_index[-i]; @@ -1425,9 +1421,8 @@ static int create_album_index(void) pf_idx.album_index = tmp_album; /* move buf ptr to end of album_index */ buf = pf_idx.album_index + pf_idx.album_ct; - ALIGN_BUFFER(buf, buf_size, 4); -/* Assign indices */ + /* Assign indices */ draw_splashscreen(buf, buf_size); draw_progressbar(0, pf_idx.album_ct, "Assigning Albums"); for (j = 0; j < pf_idx.album_ct; j++) @@ -1541,7 +1536,6 @@ retry_artist_lookup: } } - ALIGN_BUFFER(buf, buf_size, 4); pf_idx.buf = buf; pf_idx.buf_sz = buf_size; pf_idx.artist_index = 0; @@ -1618,7 +1612,6 @@ static int load_album_index(void){ //rb->lseek(fr, sizeof(data) + 1, SEEK_SET); /* artist names */ - ALIGN_BUFFER(buf, buf_size, 4); if (read2buf(fr, buf, data.artist_len) == 0) goto failure; @@ -1627,7 +1620,6 @@ static int load_album_index(void){ buf_size -= data.artist_len; /* album names */ - ALIGN_BUFFER(buf, buf_size, 4); if (read2buf(fr, buf, data.album_len) == 0) goto failure; @@ -1636,7 +1628,6 @@ static int load_album_index(void){ buf_size -= data.album_len; /* index of album names */ - ALIGN_BUFFER(buf, buf_size, 4); if (read2buf(fr, buf, album_idx_sz) == 0) goto failure; @@ -4286,8 +4277,6 @@ static int pictureflow_main(const char* selected_file) init_scroll_lines(); init_reflect_table(); - ALIGN_BUFFER(pf_idx.buf, pf_idx.buf_sz, 4); - /*Scan will trigger when no file is found or the option was activated*/ if ((pf_cfg.cache_version != CACHE_VERSION)||(load_album_index() < 0)){ rb->splash(HZ/2,"Creating index, please wait"); @@ -4312,23 +4301,20 @@ static int pictureflow_main(const char* selected_file) return PLUGIN_ERROR; } - ALIGN_BUFFER(pf_idx.buf, pf_idx.buf_sz, 4); - number_of_slides = pf_idx.album_ct; - - size_t aa_bufsz = ALIGN_DOWN(pf_idx.buf_sz / 4, 0x4); + number_of_slides = pf_idx.album_ct; + size_t aa_bufsz = pf_idx.buf_sz / 4 + sizeof(long) - 1; if (aa_bufsz < DISPLAY_WIDTH * DISPLAY_HEIGHT * sizeof(pix_t)) { error_wait("Not enough memory for album art cache"); return PLUGIN_ERROR; } - pf_idx.buf_sz -= aa_bufsz; - ALIGN_BUFFER(pf_idx.buf, pf_idx.buf_sz, 4); + ALIGN_BUFFER(pf_idx.buf, pf_idx.buf_sz, sizeof(long)); aa_cache.buf = (char*) pf_idx.buf; aa_cache.buf_sz = aa_bufsz; pf_idx.buf += aa_bufsz; - ALIGN_BUFFER(pf_idx.buf, pf_idx.buf_sz, 4); + pf_idx.buf_sz -= aa_bufsz; if (!create_empty_slide(pf_cfg.cache_version != CACHE_VERSION)) { config_save(CACHE_REBUILD); diff --git a/apps/recorder/jpeg_load.c b/apps/recorder/jpeg_load.c index 5b287aff75..34d543b56e 100644 --- a/apps/recorder/jpeg_load.c +++ b/apps/recorder/jpeg_load.c @@ -2024,7 +2024,7 @@ int clip_jpeg_fd(int fd, #else struct jpeg *p_jpeg = (struct jpeg*)bm->data; int tmp_size = maxsize; - ALIGN_BUFFER(p_jpeg, tmp_size, sizeof(int)); + ALIGN_BUFFER(p_jpeg, tmp_size, sizeof(long)); /* not enough memory for our struct jpeg */ if ((size_t)tmp_size < sizeof(struct jpeg)) return -1; @@ -2133,7 +2133,7 @@ int clip_jpeg_fd(int fd, char *buf_end = (char *)bm->data + maxsize; maxsize = buf_end - buf_start; #ifndef JPEG_FROM_MEM - ALIGN_BUFFER(buf_start, maxsize, sizeof(uint32_t)); + ALIGN_BUFFER(buf_start, maxsize, sizeof(long)); if (maxsize < (int)sizeof(struct jpeg)) return -1; memmove(buf_start, p_jpeg, sizeof(struct jpeg)); diff --git a/apps/tagcache.c b/apps/tagcache.c index c18380854e..b6d15e7a1f 100644 --- a/apps/tagcache.c +++ b/apps/tagcache.c @@ -2258,17 +2258,12 @@ static int tempbuf_sort(int fd) while (idlist->next != NULL) idlist = idlist->next; + ALIGN_BUFFER(tempbuf_pos, tempbuf_left, alignof(struct tempbuf_id_list)); tempbuf_left -= sizeof(struct tempbuf_id_list); - if (tempbuf_left - 4 < 0) + if (tempbuf_left < 0) return -1; idlist->next = (struct tempbuf_id_list *)&tempbuf[tempbuf_pos]; - if (tempbuf_pos & 0x03) - { - tempbuf_pos = (tempbuf_pos & ~0x03) + 0x04; - tempbuf_left -= 3; - idlist->next = (struct tempbuf_id_list *)&tempbuf[tempbuf_pos]; - } tempbuf_pos += sizeof(struct tempbuf_id_list); idlist = idlist->next; diff --git a/lib/skin_parser/skin_buffer.c b/lib/skin_parser/skin_buffer.c index d18122ef20..021746ba82 100644 --- a/lib/skin_parser/skin_buffer.c +++ b/lib/skin_parser/skin_buffer.c @@ -80,8 +80,8 @@ void* skin_buffer_alloc(size_t size) { void *retval = NULL; #endif - /* 32-bit aligned */ - size = (size + 3) & ~3; + /* align to long which is enough for most types */ + size = (size + sizeof(long) - 1) & ~(sizeof(long) - 1); if (size > skin_buffer_freespace()) { skin_error(MEMORY_LIMIT_EXCEEDED, NULL); -- cgit v1.2.3