Claudio Jeker <claudio@openbsd.org> writes:

> On Sun, Aug 16, 2026 at 10:04:25AM -0600, Kyle Ackerman wrote:
>> Hello,
>> 
>> Recently, in order to access the data of an imsg, I have ran into an
>> issue replacing a memcpy(3) call with a imsg_get_data(3) call, which
>> turned out to not just be plug and play.  The conditions needed for this
>> are specifying a size in the imsg_get_data(3) that is less than the
>> internally allocated buffer size.  For example the specific situation I am
>> working with is a struct of size 16 bytes that I am pulling out of a
>> imsg with an internally allocated buffer of 64 bytes.
>> 
>> imsg_get_data(struct imsg *imsg, void *data, size_t len)
>> {
>>         if (len == 0) {
>>                 errno = EINVAL;
>>                 return (-1);
>>         }
>>         if (ibuf_size(imsg->buf) != len) { // Fails here
>>                 errno = EBADMSG;
>>                 return (-1);
>>         }
>>         return ibuf_get(imsg->buf, data, len);
>> }
>> 
>> I am not entirely sure why we have the constraint that requested length
>> in the has to be exactly the size of the imsg->buf.  Having to know the
>> internally allocated buffer size in order to use the imsg_get_data IMO
>> makes the imsg struct less opaque.  Maybe there is a better way we can
>> do this? I don't see the other imsg_get_* functions preforming this
>> check.
>> 
>> The above calls ibuf_get(3), which does the bounds checking:
>> 
>> ibuf_get(struct ibuf *buf, void *data, size_t len)
>> {
>>         if (ibuf_size(buf) < len) {
>>                 errno = EBADMSG;
>>                 return (-1);
>>         }
>> 
>>         memcpy(data, ibuf_data(buf), len);
>>         buf->rpos += len;
>>         return (0);
>> }
>> 
>> 
>> 
>> I propose this diff, which removes the check that the requested length
>> of data is equal to the size of the allocated buffer.  This would: rely
>> on the bounds check preformed in the ibuf_get(3) functions, removes a
>> ibuf_size(3) call, and allow the use of imsg_get_data(3) without really
>> knowing anything about the imsg struct.
>> 
>> diff /usr/src
>> path + /usr/src
>> commit - 63d30f6f93ff9faed796cfea1af89df9fa4894de
>> blob - b8d93d4ece10b8ae89b8c469d17d179126f22b3b
>> file + lib/libutil/imsg.c
>> --- lib/libutil/imsg.c
>> +++ lib/libutil/imsg.c
>> @@ -194,10 +194,6 @@ imsg_get_data(struct imsg *imsg, void *data, size_t le
>>  		errno = EINVAL;
>>  		return (-1);
>>  	}
>> -	if (ibuf_size(imsg->buf) != len) {
>> -		errno = EBADMSG;
>> -		return (-1);
>> -	}
>>  	return ibuf_get(imsg->buf, data, len);
>>  }
>>  
>> 
>> 
>> Thoughts/Comments/Suggestions? I can also share more about the specific
>> use-case I am running into if that makes a difference.
>> 
>
> imsg_get_data is exactly designed to be strict:
>      imsg_get_data() can be used if the structure of the payload is
>      known and can be extracted in one go.
> So no this diff is not acceptable.
>
> If this is not the case like here then use imsg_get_buf() and
> imsg_get_strbuf() these functions advance the buffer but then you should
> also check that at the end all data was consumed using
> 	imsg_get_len(&imsg) == 0
> For even more complex cases imsg_get_ibuf() can be used, it returns an
> ibuf and can be used by all getters from the ibuf_get family.



Understood, thank you for the clarification.