Re: [PATCH] simfs: Fix reads beyond the first block
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 ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH] simfs: Fix reads beyond the first block
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 ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH] simfs: Fix reads beyond the first block
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 ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
Re: Documentation on when to free structures in/after callbacks
Thanks for that Denis - that helps me get it much clearer in my head! Cheers, Alex ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
Re: Documentation on when to free structures in/after callbacks
Hi Alex,
On 7/30/21 9:24 AM, ajlen...@dynamicdevices.co.uk wrote:
Thanks Denis that helps. I see I need to learn a bit and get my head around
this.
For example I find this really confusing:
cbd = req_cb_data_ref(cbd);
if (g_at_chat_send(nmd->chat, "AT+CESQ", cesq_prefix,
cesq_cb, cbd, req_cb_data_unref) == 0) {
CALLBACK_WITH_FAILURE(cbd->cb, cbd->data);
req_cb_data_unref(cbd);
}
This does some kind of allocation - ok
This makes the call - ok fine
Provides the unref function into the call - ok so far
So in this particular case, g_at_chat_send() returning 0 means the command could
not be scheduled for whatever reason (usually the tty port is dead). By
convention, failures do not side-effect. So you have to free any allocated
resources manually. Hence the unref here.
But then I'm thinking if the call _succeeds_ our unref gets called?
Yes. destroy callback is called in every circumstance once g_at_chat_send()
returns a success. So it will get called after cesq_cb(), or if the request is
cleaned up early (cesq_cb() never called)
But if our call _fails_ then we have to do the unref ourselves?
Right, the 'no-side-effect' part applies.
Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
Re: Documentation on when to free structures in/after callbacks
Thanks Denis that helps. I see I need to learn a bit and get my head around
this.
For example I find this really confusing:
cbd = req_cb_data_ref(cbd);
if (g_at_chat_send(nmd->chat, "AT+CESQ", cesq_prefix,
cesq_cb, cbd, req_cb_data_unref) == 0) {
CALLBACK_WITH_FAILURE(cbd->cb, cbd->data);
req_cb_data_unref(cbd);
}
This does some kind of allocation - ok
This makes the call - ok fine
Provides the unref function into the call - ok so far
But then I'm thinking if the call _succeeds_ our unref gets called?
But if our call _fails_ then we have to do the unref ourselves?
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
Re: Documentation on when to free structures in/after callbacks
Hi Alex,
On 7/30/21 8:21 AM, ajlen...@dynamicdevices.co.uk wrote:
I'm not sure what you mean? What exactly do you think needs freeing?
Strange. I am lookinga at the drivers and sometimes unref functions are
provided into other functions and sometimes not
So you're talking about callbacks in the driver itself, not actually crossing
the core<->driver boundary.
There aren't really any docs, but g_at_chat acts just like any other GLib based
library (or many main-loop driven C libraries for that matter), so pretty much
the same rules apply. I can give you some hints...
if (g_at_chat_send(nmd->chat, "AT+QENG=\"servingcell\"", qeng_prefix,
qeng_cb, cbd, req_cb_data_unref) == 0) {
Under the hood, g_at_chat_send puts a request on the queue. And the request
might not be processed until some time later. It can also be interrupted and
cleaned up without the callback being called (if the hardware gets pulled for
example). So if you're passing any allocated userdata, you need to provide a
'destroy' method to de-allocate it, otherwise it will leak.
In your example, cbd is userdata and req_cb_data_unref is the destroy method,
respectively.
Oh, and req_cb_data is a common pattern. It is used where you need to run
several commands in sequence (think of it as a transaction) and the allocated
userdata needs to be cleaned up even if the transaction is interrupted.
Then these CALLBACK_WITH_FAILURE and CALLBACK_WITH_SUCCESS calls sometimes seem
to have free functions after them and sometimes not.
There is definitely a reason why they do. Hopefully the explanation above
helps.
Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
Re: Documentation on when to free structures in/after callbacks
>I'm not sure what you mean? What exactly do you think needs freeing?
Strange. I am lookinga at the drivers and sometimes unref functions are
provided into other functions and sometimes not
if (g_at_chat_send(nmd->chat, "AT+QENG=\"servingcell\"", qeng_prefix,
qeng_cb, cbd, req_cb_data_unref) == 0) {
Then these CALLBACK_WITH_FAILURE and CALLBACK_WITH_SUCCESS calls sometimes seem
to have free functions after them and sometimes not.
I guess I am not understanding then Will continue look at this. I just
thought there might be sometime in docs about the flow.
Regards,
-Alex
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
[PATCH] simfs: Fix reads beyond the first block
---
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
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
