Hi Aaron, On Fri, 2026-03-13 at 18:18 -0400, Aaron Merey wrote: > Similar to commit c460e2088, gelf_update_move does not distinguish > between ELFCLASS32 and ELFCLASS64 binaries. > > The code assumes that Elf32_Move and Elf64_Move structs are the same > size but they are not. The m_info and m_poffset fields have type > uint32_t for Elf32_Move but uint64_t for Elf64_Move. > > Fix this by handling ELFCLASS32 and ELFCLASS64 cases separately.
Looks good to me. Just one comment (on existing code) that we don't have to fix now, but should think about. > Signed-off-by: Aaron Merey <[email protected]> > --- > libelf/gelf_update_move.c | 70 +++++++++++++++++++++++++++++---------- > 1 file changed, 53 insertions(+), 17 deletions(-) > > diff --git a/libelf/gelf_update_move.c b/libelf/gelf_update_move.c > index 4190ee30..2b12c7eb 100644 > --- a/libelf/gelf_update_move.c > +++ b/libelf/gelf_update_move.c > @@ -41,37 +41,73 @@ > int > gelf_update_move (Elf_Data *data, int ndx, GElf_Move *src) > { > + int result = 0; > Elf_Data_Scn *data_scn = (Elf_Data_Scn *) data; > + Elf *elf; > > if (data == NULL) > - return 0; > + return result; OK. > - /* The types for 32 and 64 bit are the same. Lucky us. */ > - assert (sizeof (GElf_Move) == sizeof (Elf32_Move)); > - assert (sizeof (GElf_Move) == sizeof (Elf64_Move)); > - > - /* Check whether we have to resize the data buffer. */ > - if (INVALID_NDX (ndx, GElf_Move, &data_scn->d)) > + if (unlikely (data->d_type != ELF_T_MOVE)) > { > - __libelf_seterrno (ELF_E_INVALID_INDEX); > - return 0; > + /* The type of the data better should match. */ > + __libelf_seterrno (ELF_E_DATA_MISMATCH); > + return result; > } OK. > > - if (unlikely (data_scn->d.d_type != ELF_T_MOVE)) > + elf = data_scn->s->elf; > + rwlock_wrlock (elf->lock); We take the elf->lock here, but only use it for getting the elf->class. That better not change cross calls manipulating the Elf_Data. Everything else just depends on the provided Elf_Data. And I think we really should just require there is no concurrent manipulation of that (or require all manipulations to also take the elf->lock ?) > + if (elf->class == ELFCLASS32) > { > - /* The type of the data better should match. */ > - __libelf_seterrno (ELF_E_DATA_MISMATCH); > - return 0; > + Elf32_Move *move; > + > + /* Check whether input values are too large for 32 bit conversion. */ > + if (unlikely (src->m_poffset > 0xffffffffull) > + || unlikely (GELF_M_SYM (src->m_info) > 0xffffff) > + || unlikely (GELF_M_SIZE (src->m_info) > 0xff)) > + { > + __libelf_seterrno (ELF_E_INVALID_DATA); > + goto out; > + } > + > + /* Check whether we have to resize the data buffer. */ > + if (INVALID_NDX (ndx, Elf32_Move, data)) > + { > + __libelf_seterrno (ELF_E_INVALID_INDEX); > + goto out; > + } > + > + move = &((Elf32_Move *) data->d_buf)[ndx]; > + > + move->m_value = src->m_value; > + move->m_info = ELF32_M_INFO (GELF_M_SYM (src->m_info), > + GELF_M_SIZE (src->m_info)); > + move->m_poffset = src->m_poffset; > + move->m_repeat = src->m_repeat; > + move->m_stride = src->m_stride; > } OK. > + else > + { > + eu_static_assert (sizeof (GElf_Move) == sizeof (Elf64_Move)); Yeah, static assert is good here. > - rwlock_wrlock (data_scn->s->elf->lock); > + /* Check whether we have to resize the data buffer. */ > + if (INVALID_NDX (ndx, GElf_Move, data)) The comment is a little odd (although I have seen it in other place too. We don't resize the data buffer, we just report an error if a too big index is used. See also above for the 32 variant. > + { > + __libelf_seterrno (ELF_E_INVALID_INDEX); > + goto out; > + } > + > + ((Elf64_Move *) data->d_buf)[ndx] = *src; > + } > > - ((GElf_Move *) data_scn->d.d_buf)[ndx] = *src; > + result = 1; > > /* Mark the section as modified. */ > data_scn->s->flags |= ELF_F_DIRTY; > > - rwlock_unlock (data_scn->s->elf->lock); > + out: > + rwlock_unlock (elf->lock); > > - return 1; > + return result; > } OK. Same comment about the (unnecessary?) unlock of the elf->lock. Cheers, Mark
