Message ID | 20220609014715.1353-1-rosenp@gmail.com |
---|---|
State | New |
Headers | show |
Series | uqmi: fix compilation with GCC12 | expand |
The 'dangling pointer' issue can be fix without using malloc(). --- a/dev.c 2022-05-04 02:18:17.000000000 +0200 +++ b/dev.c 2022-06-11 08:48:21.185567953 +0200 @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev * int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) { bool complete = false; + bool *c; bool cancelled; if (!req->pending) @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi uloop_cancelled = cancelled; } - if (req->complete == &complete) - req->complete = NULL; + c = req->complete; + req->complete = NULL; + if (c != &complete) + req->complete = c; return req->ret; } Am 09.06.2022 um 03:47 schrieb Rosen Penev: > GCC12 doesn't seem to see that the completed member gets nulled. Use > malloc to work around this. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > commands-nas.c | 2 +- > dev.c | 9 +++++---- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/commands-nas.c b/commands-nas.c > index 476cd61..9c0a626 100644 > --- a/commands-nas.c > +++ b/commands-nas.c > @@ -710,7 +710,7 @@ static void > cmd_nas_get_cell_location_info_cb(struct qmi_dev *qmi, struct qmi_request *req, struct qmi_msg *msg) > { > struct qmi_nas_get_cell_location_info_response res; > - void *c, *t, *cell, *freq; > + void *c = NULL, *t, *cell, *freq; > int i, j; > > qmi_parse_nas_get_cell_location_info_response(msg, &res); > diff --git a/dev.c b/dev.c > index bd10207..b1cf2a3 100644 > --- a/dev.c > +++ b/dev.c > @@ -205,7 +205,7 @@ void qmi_request_cancel(struct qmi_dev *qmi, struct qmi_request *req) > > int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) > { > - bool complete = false; > + bool *complete = malloc(sizeof(bool)); > bool cancelled; > > if (!req->pending) > @@ -214,8 +214,8 @@ int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) > if (req->complete) > *req->complete = true; > > - req->complete = &complete; > - while (!complete) { > + req->complete = complete; > + while (!*complete) { > cancelled = uloop_cancelled; > uloop_cancelled = false; > uloop_run(); > @@ -226,9 +226,10 @@ int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) > uloop_cancelled = cancelled; > } > > - if (req->complete == &complete) > + if (req->complete == complete) > req->complete = NULL; > > + free(complete); > return req->ret; > } >
e9hack <e9hack@gmail.com> writes: > The 'dangling pointer' issue can be fix without using malloc(). > > --- a/dev.c 2022-05-04 02:18:17.000000000 +0200 > +++ b/dev.c 2022-06-11 08:48:21.185567953 +0200 > @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev * > int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) > { > bool complete = false; > + bool *c; > bool cancelled; > if (!req->pending) > @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi > uloop_cancelled = cancelled; > } > - if (req->complete == &complete) > - req->complete = NULL; > + c = req->complete; > + req->complete = NULL; > + if (c != &complete) > + req->complete = c; > return req->ret; > } How about just fixing GCC instead? Having all sorts of funny and pointless code just to avoid bogus compiler warnings is kind of stupid, isn't it? If GCC can't do better that this, then it's much better to disable that warning. Bjørn
On 6/11/22 16:47, Bjørn Mork wrote: > e9hack <e9hack@gmail.com> writes: > >> The 'dangling pointer' issue can be fix without using malloc(). >> >> --- a/dev.c 2022-05-04 02:18:17.000000000 +0200 >> +++ b/dev.c 2022-06-11 08:48:21.185567953 +0200 >> @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev * >> int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) >> { >> bool complete = false; >> + bool *c; >> bool cancelled; >> if (!req->pending) >> @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi >> uloop_cancelled = cancelled; >> } >> - if (req->complete == &complete) >> - req->complete = NULL; >> + c = req->complete; >> + req->complete = NULL; >> + if (c != &complete) >> + req->complete = c; >> return req->ret; >> } > > How about just fixing GCC instead? Having all sorts of funny and > pointless code just to avoid bogus compiler warnings is kind of stupid, > isn't it? > > If GCC can't do better that this, then it's much better to disable that > warning. GCC complains about this code because the pointer is only removed conditionally ------------------------------------------- if (req->complete == &complete) req->complete = NULL; ------------------------------------------- https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229 When you make this part unconditionally it does not complain any more. Is it possible that something changes the req->complete pointer address in between? Hauke
Hauke Mehrtens <hauke@hauke-m.de> writes: > GCC complains about this code because the pointer is only removed > conditionally > ------------------------------------------- > if (req->complete == &complete) > req->complete = NULL; > ------------------------------------------- > https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229 Yes, but that condition is testing for precisely the dangling pointer.GCC complains about. If it is false, then there is no problem. If it is true, then the pointer is removed and there is no problem. If GCC is unable to figure that out, then it should not try. Bjørn
On Sun, Aug 21, 2022 at 7:54 AM Hauke Mehrtens <hauke@hauke-m.de> wrote: > > On 6/11/22 16:47, Bjørn Mork wrote: > > e9hack <e9hack@gmail.com> writes: > > > >> The 'dangling pointer' issue can be fix without using malloc(). > >> > >> --- a/dev.c 2022-05-04 02:18:17.000000000 +0200 > >> +++ b/dev.c 2022-06-11 08:48:21.185567953 +0200 > >> @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev * > >> int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) > >> { > >> bool complete = false; > >> + bool *c; > >> bool cancelled; > >> if (!req->pending) > >> @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi > >> uloop_cancelled = cancelled; > >> } > >> - if (req->complete == &complete) > >> - req->complete = NULL; > >> + c = req->complete; > >> + req->complete = NULL; > >> + if (c != &complete) > >> + req->complete = c; > >> return req->ret; > >> } > > > > How about just fixing GCC instead? Having all sorts of funny and > > pointless code just to avoid bogus compiler warnings is kind of stupid, > > isn't it? > > > > If GCC can't do better that this, then it's much better to disable that > > warning. > > GCC complains about this code because the pointer is only removed > conditionally > ------------------------------------------- > if (req->complete == &complete) > req->complete = NULL; > ------------------------------------------- > https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229 > > When you make this part unconditionally it does not complain any more. > > Is it possible that something changes the req->complete pointer address > in between? this is still an issue. > > Hauke
On 9/28/22 04:22, Rosen Penev wrote: > On Sun, Aug 21, 2022 at 7:54 AM Hauke Mehrtens <hauke@hauke-m.de> wrote: >> >> On 6/11/22 16:47, Bjørn Mork wrote: >>> e9hack <e9hack@gmail.com> writes: >>> >>>> The 'dangling pointer' issue can be fix without using malloc(). >>>> >>>> --- a/dev.c 2022-05-04 02:18:17.000000000 +0200 >>>> +++ b/dev.c 2022-06-11 08:48:21.185567953 +0200 >>>> @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev * >>>> int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) >>>> { >>>> bool complete = false; >>>> + bool *c; >>>> bool cancelled; >>>> if (!req->pending) >>>> @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi >>>> uloop_cancelled = cancelled; >>>> } >>>> - if (req->complete == &complete) >>>> - req->complete = NULL; >>>> + c = req->complete; >>>> + req->complete = NULL; >>>> + if (c != &complete) >>>> + req->complete = c; >>>> return req->ret; >>>> } >>> >>> How about just fixing GCC instead? Having all sorts of funny and >>> pointless code just to avoid bogus compiler warnings is kind of stupid, >>> isn't it? >>> >>> If GCC can't do better that this, then it's much better to disable that >>> warning. >> >> GCC complains about this code because the pointer is only removed >> conditionally >> ------------------------------------------- >> if (req->complete == &complete) >> req->complete = NULL; >> ------------------------------------------- >> https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229 >> >> When you make this part unconditionally it does not complain any more. >> >> Is it possible that something changes the req->complete pointer address >> in between? > this is still an issue. >> >> Hauke Hi, Is it possible to deactivate the error and make it only a warning which we can ignore? I would deactivate this error for the complete application and add a comment to it that this is a workaround for GCC 12. Did someone create a ticket at GCC for this problem? I am aware of the one for elfutils only. Hauke
On Thu, Sep 29, 2022 at 7:45 AM Hauke Mehrtens <hauke@hauke-m.de> wrote: > > On 9/28/22 04:22, Rosen Penev wrote: > > On Sun, Aug 21, 2022 at 7:54 AM Hauke Mehrtens <hauke@hauke-m.de> wrote: > >> > >> On 6/11/22 16:47, Bjørn Mork wrote: > >>> e9hack <e9hack@gmail.com> writes: > >>> > >>>> The 'dangling pointer' issue can be fix without using malloc(). > >>>> > >>>> --- a/dev.c 2022-05-04 02:18:17.000000000 +0200 > >>>> +++ b/dev.c 2022-06-11 08:48:21.185567953 +0200 > >>>> @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev * > >>>> int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) > >>>> { > >>>> bool complete = false; > >>>> + bool *c; > >>>> bool cancelled; > >>>> if (!req->pending) > >>>> @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi > >>>> uloop_cancelled = cancelled; > >>>> } > >>>> - if (req->complete == &complete) > >>>> - req->complete = NULL; > >>>> + c = req->complete; > >>>> + req->complete = NULL; > >>>> + if (c != &complete) > >>>> + req->complete = c; > >>>> return req->ret; > >>>> } > >>> > >>> How about just fixing GCC instead? Having all sorts of funny and > >>> pointless code just to avoid bogus compiler warnings is kind of stupid, > >>> isn't it? > >>> > >>> If GCC can't do better that this, then it's much better to disable that > >>> warning. > >> > >> GCC complains about this code because the pointer is only removed > >> conditionally > >> ------------------------------------------- > >> if (req->complete == &complete) > >> req->complete = NULL; > >> ------------------------------------------- > >> https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229 > >> > >> When you make this part unconditionally it does not complain any more. > >> > >> Is it possible that something changes the req->complete pointer address > >> in between? > > this is still an issue. > >> > >> Hauke > Hi, > > Is it possible to deactivate the error and make it only a warning which > we can ignore? > I would deactivate this error for the complete application and add a > comment to it that this is a workaround for GCC 12. > > Did someone create a ticket at GCC for this problem? I am aware of the > one for elfutils only. What's wrong with working around it? It's not difficult. > > Hauke
diff --git a/commands-nas.c b/commands-nas.c index 476cd61..9c0a626 100644 --- a/commands-nas.c +++ b/commands-nas.c @@ -710,7 +710,7 @@ static void cmd_nas_get_cell_location_info_cb(struct qmi_dev *qmi, struct qmi_request *req, struct qmi_msg *msg) { struct qmi_nas_get_cell_location_info_response res; - void *c, *t, *cell, *freq; + void *c = NULL, *t, *cell, *freq; int i, j; qmi_parse_nas_get_cell_location_info_response(msg, &res); diff --git a/dev.c b/dev.c index bd10207..b1cf2a3 100644 --- a/dev.c +++ b/dev.c @@ -205,7 +205,7 @@ void qmi_request_cancel(struct qmi_dev *qmi, struct qmi_request *req) int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) { - bool complete = false; + bool *complete = malloc(sizeof(bool)); bool cancelled; if (!req->pending) @@ -214,8 +214,8 @@ int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) if (req->complete) *req->complete = true; - req->complete = &complete; - while (!complete) { + req->complete = complete; + while (!*complete) { cancelled = uloop_cancelled; uloop_cancelled = false; uloop_run(); @@ -226,9 +226,10 @@ int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req) uloop_cancelled = cancelled; } - if (req->complete == &complete) + if (req->complete == complete) req->complete = NULL; + free(complete); return req->ret; }
GCC12 doesn't seem to see that the completed member gets nulled. Use malloc to work around this. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- commands-nas.c | 2 +- dev.c | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-)