* [PATCH] simfs: Fix reads beyond the first block
@ 2021-07-30 12:07 Slava Monich
2021-07-30 15:37 ` Denis Kenzior
0 siblings, 1 reply; 4+ messages in thread
From: Slava Monich @ 2021-07-30 12:07 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]
---
src/simfs.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/src/simfs.c b/src/simfs.c
index 3d4f6283..cf770265 100644
--- a/src/simfs.c
+++ b/src/simfs.c
@@ -383,18 +383,18 @@ static void sim_fs_op_read_block_cb(const struct ofono_error *error,
}
start_block = op->offset / 256;
- end_block = (op->offset + (op->num_bytes - 1)) / 256;
+ end_block = op->num_bytes ? (op->offset + op->num_bytes - 1) / 256 :
+ start_block;
if (op->current == start_block) {
bufoff = 0;
dataoff = op->offset % 256;
- tocopy = MIN(256 - op->offset % 256,
- op->num_bytes - op->current * 256);
+ tocopy = MIN(256 - dataoff, op->num_bytes);
} else {
- bufoff = (op->current - start_block - 1) * 256 +
+ bufoff = (op->current - start_block) * 256 -
op->offset % 256;
dataoff = 0;
- tocopy = MIN(256, op->num_bytes - op->current * 256);
+ tocopy = MIN(256, op->num_bytes - bufoff);
}
DBG("bufoff: %d, dataoff: %d, tocopy: %d",
@@ -463,13 +463,12 @@ static gboolean sim_fs_op_read_block(gpointer user_data)
bufoff = 0;
seekoff = SIM_CACHE_HEADER_SIZE + op->current * 256 +
op->offset % 256;
- toread = MIN(256 - op->offset % 256,
- op->num_bytes - op->current * 256);
+ toread = MIN(256 - op->offset % 256, op->num_bytes);
} else {
- bufoff = (op->current - start_block - 1) * 256 +
+ bufoff = (op->current - start_block) * 256 -
op->offset % 256;
seekoff = SIM_CACHE_HEADER_SIZE + op->current * 256;
- toread = MIN(256, op->num_bytes - op->current * 256);
+ toread = MIN(256, op->num_bytes - bufoff);
}
DBG("bufoff: %d, seekoff: %d, toread: %d",
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] simfs: Fix reads beyond the first block
2021-07-30 12:07 [PATCH] simfs: Fix reads beyond the first block Slava Monich
@ 2021-07-30 15:37 ` Denis Kenzior
2021-07-30 15:52 ` Slava Monich
0 siblings, 1 reply; 4+ messages in thread
From: Denis Kenzior @ 2021-07-30 15:37 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 792 bytes --]
Hi Slava,
On 7/30/21 7:07 AM, Slava Monich wrote:
> ---
> src/simfs.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
Funny how long this bug has been lurking around.
> diff --git a/src/simfs.c b/src/simfs.c
> index 3d4f6283..cf770265 100644
> --- a/src/simfs.c
> +++ b/src/simfs.c
> @@ -383,18 +383,18 @@ static void sim_fs_op_read_block_cb(const struct ofono_error *error,
> }
>
> start_block = op->offset / 256;
> - end_block = (op->offset + (op->num_bytes - 1)) / 256;
> + end_block = op->num_bytes ? (op->offset + op->num_bytes - 1) / 256 :
> + start_block;
>
Curious why this is needed? op->num_bytes should never be zero since it gets
set to the file length?
Rest looks good to me.
Regards,
-Denis
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] simfs: Fix reads beyond the first block
2021-07-30 15:37 ` Denis Kenzior
@ 2021-07-30 15:52 ` Slava Monich
2021-07-30 16:08 ` Denis Kenzior
0 siblings, 1 reply; 4+ messages in thread
From: Slava Monich @ 2021-07-30 15:52 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]
On 30/07/2021 18.37, Denis Kenzior wrote:
> Hi Slava,
>
> On 7/30/21 7:07 AM, Slava Monich wrote:
>> ---
>> src/simfs.c | 17 ++++++++---------
>> 1 file changed, 8 insertions(+), 9 deletions(-)
>>
>
> Funny how long this bug has been lurking around.
>
Until we finally had a crash on reading an icon or something out of SIM.
Which most SIMs apparently don't have or else it would've been noticed
earlier.
>> diff --git a/src/simfs.c b/src/simfs.c
>> index 3d4f6283..cf770265 100644
>> --- a/src/simfs.c
>> +++ b/src/simfs.c
>> @@ -383,18 +383,18 @@ static void sim_fs_op_read_block_cb(const
>> struct ofono_error *error,
>> }
>> start_block = op->offset / 256;
>> - end_block = (op->offset + (op->num_bytes - 1)) / 256;
>> + end_block = op->num_bytes ? (op->offset + op->num_bytes - 1) /
>> 256 :
>> + start_block;
>
> Curious why this is needed? op->num_bytes should never be zero since
> it gets set to the file length?
>
I admit that it's a bit paranoid, but op->num_bytes is assigned without
checking and I figured that it wouldn't hurt to do a check here. Feel
free to drop this part if it looks like too much of an overkill to you.
> Rest looks good to me.
>
> Regards,
> -Denis
Cheers,
-Slava
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] simfs: Fix reads beyond the first block
2021-07-30 15:52 ` Slava Monich
@ 2021-07-30 16:08 ` Denis Kenzior
0 siblings, 0 replies; 4+ messages in thread
From: Denis Kenzior @ 2021-07-30 16:08 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]
Hi Slava,
On 7/30/21 10:52 AM, Slava Monich wrote:
> On 30/07/2021 18.37, Denis Kenzior wrote:
>> Hi Slava,
>>
>> On 7/30/21 7:07 AM, Slava Monich wrote:
>>> ---
>>> src/simfs.c | 17 ++++++++---------
>>> 1 file changed, 8 insertions(+), 9 deletions(-)
>>>
>>
>> Funny how long this bug has been lurking around.
>>
>
> Until we finally had a crash on reading an icon or something out of SIM. Which
> most SIMs apparently don't have or else it would've been noticed earlier.
>
Figured :)
>
>>> diff --git a/src/simfs.c b/src/simfs.c
>>> index 3d4f6283..cf770265 100644
>>> --- a/src/simfs.c
>>> +++ b/src/simfs.c
>>> @@ -383,18 +383,18 @@ static void sim_fs_op_read_block_cb(const struct
>>> ofono_error *error,
>>> }
>>> start_block = op->offset / 256;
>>> - end_block = (op->offset + (op->num_bytes - 1)) / 256;
>>> + end_block = op->num_bytes ? (op->offset + op->num_bytes - 1) / 256 :
>>> + start_block;
>>
>> Curious why this is needed? op->num_bytes should never be zero since it gets
>> set to the file length?
>>
>
> I admit that it's a bit paranoid, but op->num_bytes is assigned without checking
> and I figured that it wouldn't hurt to do a check here. Feel free to drop this
> part if it looks like too much of an overkill to you.
>
I'm all for being paranoid, but there's not much sense doing a round-trip to the
SIM for a 0 byte read. So I rather we catch this elsewhere.
There's some sanity checking of num_bytes being 0 once the file info has been
obtained. It should be reset to file length if it is zero, which is generally
what we want for simple binary files.
Invocations of ofono_sim_read_bytes() should probably sanity check the length if
they want to be fully paranoid.
I went ahead and applied this patch without this particular chunk.
Regards,
-Denis
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-07-30 16:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-30 12:07 [PATCH] simfs: Fix reads beyond the first block Slava Monich
2021-07-30 15:37 ` Denis Kenzior
2021-07-30 15:52 ` Slava Monich
2021-07-30 16:08 ` Denis Kenzior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.