Re: [PATCH 1/1] fs: fix smh_fs_read_at()

On 5/18/23 17:17, Sean Anderson wrote:

On 5/17/23 06:23, Heinrich Schuchardt wrote:

...

The return value of smh_flen() is written to size and not to ret. But ret is checked. We can avoid calling smh_flen() by setting maxsize to LONG_MAX if it is not set yet.

Check input parameters.

Fixes: f676b45151c3 ("fs: Add semihosting filesystem") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

---

fs/semihostingfs.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/fs/semihostingfs.c b/fs/semihostingfs.c index 96eb3349a2..8a7d4da884 100644 --- a/fs/semihostingfs.c +++ b/fs/semihostingfs.c @@ -25,6 +25,9 @@ static int smh_fs_read_at(const char *filename, loff_t pos, void *buffer, { long fd, size, ret;

• if (pos > LONG_MAX || maxsize > LONG_MAX)

Should be ULONG_MAX. The type should really be ulong but isn't.

...

• return -EINVAL;
  

• fd = smh_open(filename, MODE_READ | MODE_BINARY); if (fd < 0) return fd;

@@ -33,15 +36,8 @@ static int smh_fs_read_at(const char *filename, loff_t pos, void *buffer, smh_close(fd); return ret; }

• if (!maxsize) {

• size = smh_flen(fd);
  

• if (ret < 0) {
  

• 	smh_close(fd);
  

• 	return size;
  

• }
  

• maxsize = size;
  

• }

• if (!maxsize)

• maxsize = LONG_MAX;
  

Same here.

...

size = smh_read(fd, buffer, maxsize);

Thanks for reviewing.

Just changing to ULONG_MAX will not work, because smh_read() returns errors as negative numbers.

We would need to change the interface of smh_read to:

int smh_read(long fd, void *memp, unsigned long *len)

Then we could return the error code and the number of bytes read separately.

https://developer.arm.com/documentation/dui0471/m/what-is-semihosting-/sys-r... describes that SYS_READ may return less bytes than requested. This does not signify that EOF has been reached but may simply reflect the behavior of the underlying operating system (cf. 'man 2 read').

So shouldn't we loop here until all bytes are read?

Best regards

Heinrich

...

smh_close(fd);

This interface is nuts, but this patch does successfully implement it...

--Sean