| Message ID | 20191023122145.14314-2-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] kernel-shark: Fix simple typo in the "File" menu. | expand |
On Wed, 23 Oct 2019 15:21:44 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > When searching for the entry, do not loop over the original list of > requests. Use a copy instead. If we loop over the original list and > no entry is found in the first element of the list, later the memory > used for this first element will leak. > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > kernel-shark/src/libkshark-collection.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/kernel-shark/src/libkshark-collection.c b/kernel-shark/src/libkshark-collection.c > index 02a014e..95fdbab 100644 > --- a/kernel-shark/src/libkshark-collection.c > +++ b/kernel-shark/src/libkshark-collection.c > @@ -622,6 +622,7 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req, > ssize_t *index) > { > const struct kshark_entry *entry = NULL; > + struct kshark_entry_request *list; Hi Yordan, I was looking at this patch in more detail, and I'm thinking that we don't need to pass in the address of the req pointer, but just the req pointer itself. The only place that I see the req pointer being modified is the failure case in map_collection_request_init() where it does: kshark_free_entry_request(*req); *req = NULL; But all callers do that free anyway. Maybe I'm missing something, but why are we passing in the pointer to the pointer of req, and not just the req pointer itself? I don't see a need to modify the pointer. Before this patch, *req is modified, but after this patch, it is not. If you pass in just "struct kshark_entry_request *req" then you don't even need to have the "list" variable, you could just use "req" because that would be a copy of the pointer. -- Steve > int req_count; > > /* > @@ -638,12 +639,10 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req, > * Loop over the list of redefined requests and search until you find > * the first matching entry. > */ > - while (*req) { > - entry = kshark_get_entry_front(*req, data, index); > + for (list = *req; list; list = list->next) { > + entry = kshark_get_entry_front(list, data, index); > if (entry) > break; > - > - *req = (*req)->next; > } > > return entry; > @@ -680,6 +679,7 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req, > ssize_t *index) > { > const struct kshark_entry *entry = NULL; > + struct kshark_entry_request *list; > int req_count; > > /* > @@ -695,12 +695,10 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req, > * Loop over the list of redefined requests and search until you find > * the first matching entry. > */ > - while (*req) { > - entry = kshark_get_entry_back(*req, data, index); > + for (list = *req; list; list = list->next) { > + entry = kshark_get_entry_back(list, data, index); > if (entry) > break; > - > - *req = (*req)->next; > } > > return entry;
On 27.11.19 г. 20:54 ч., Steven Rostedt wrote: > On Wed, 23 Oct 2019 15:21:44 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> When searching for the entry, do not loop over the original list of >> requests. Use a copy instead. If we loop over the original list and >> no entry is found in the first element of the list, later the memory >> used for this first element will leak. >> >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> kernel-shark/src/libkshark-collection.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/kernel-shark/src/libkshark-collection.c b/kernel-shark/src/libkshark-collection.c >> index 02a014e..95fdbab 100644 >> --- a/kernel-shark/src/libkshark-collection.c >> +++ b/kernel-shark/src/libkshark-collection.c >> @@ -622,6 +622,7 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req, >> ssize_t *index) >> { >> const struct kshark_entry *entry = NULL; >> + struct kshark_entry_request *list; > > Hi Yordan, > > I was looking at this patch in more detail, and I'm thinking that we > don't need to pass in the address of the req pointer, but just the req > pointer itself. The only place that I see the req pointer being > modified is the failure case in map_collection_request_init() where it > does: > > kshark_free_entry_request(*req); > *req = NULL; > > But all callers do that free anyway. > Yes, because the caller is expected to do kshark_free_entry_request(*req) at the end, here we have to set the original pointer to NULL. Otherwise we will get double free error. I think this is what I have been trying to fix, when I introduced the memory leak. And yes, I agree with you that carrying the address of the pointer through all these functions is a bit ugly. Thanks! Yordan > Maybe I'm missing something, but why are we passing in the pointer to > the pointer of req, and not just the req pointer itself? I don't see a > need to modify the pointer. > > Before this patch, *req is modified, but after this patch, it is not. > If you pass in just "struct kshark_entry_request *req" then you don't > even need to have the "list" variable, you could just use "req" because > that would be a copy of the pointer. > > -- Steve > > > >> int req_count; >> >> /* >> @@ -638,12 +639,10 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req, >> * Loop over the list of redefined requests and search until you find >> * the first matching entry. >> */ >> - while (*req) { >> - entry = kshark_get_entry_front(*req, data, index); >> + for (list = *req; list; list = list->next) { >> + entry = kshark_get_entry_front(list, data, index); >> if (entry) >> break; >> - >> - *req = (*req)->next; >> } >> >> return entry; >> @@ -680,6 +679,7 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req, >> ssize_t *index) >> { >> const struct kshark_entry *entry = NULL; >> + struct kshark_entry_request *list; >> int req_count; >> >> /* >> @@ -695,12 +695,10 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req, >> * Loop over the list of redefined requests and search until you find >> * the first matching entry. >> */ >> - while (*req) { >> - entry = kshark_get_entry_back(*req, data, index); >> + for (list = *req; list; list = list->next) { >> + entry = kshark_get_entry_back(list, data, index); >> if (entry) >> break; >> - >> - *req = (*req)->next; >> } >> >> return entry; >
diff --git a/kernel-shark/src/libkshark-collection.c b/kernel-shark/src/libkshark-collection.c index 02a014e..95fdbab 100644 --- a/kernel-shark/src/libkshark-collection.c +++ b/kernel-shark/src/libkshark-collection.c @@ -622,6 +622,7 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req, ssize_t *index) { const struct kshark_entry *entry = NULL; + struct kshark_entry_request *list; int req_count; /* @@ -638,12 +639,10 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req, * Loop over the list of redefined requests and search until you find * the first matching entry. */ - while (*req) { - entry = kshark_get_entry_front(*req, data, index); + for (list = *req; list; list = list->next) { + entry = kshark_get_entry_front(list, data, index); if (entry) break; - - *req = (*req)->next; } return entry; @@ -680,6 +679,7 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req, ssize_t *index) { const struct kshark_entry *entry = NULL; + struct kshark_entry_request *list; int req_count; /* @@ -695,12 +695,10 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req, * Loop over the list of redefined requests and search until you find * the first matching entry. */ - while (*req) { - entry = kshark_get_entry_back(*req, data, index); + for (list = *req; list; list = list->next) { + entry = kshark_get_entry_back(list, data, index); if (entry) break; - - *req = (*req)->next; } return entry;
When searching for the entry, do not loop over the original list of requests. Use a copy instead. If we loop over the original list and no entry is found in the first element of the list, later the memory used for this first element will leak. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- kernel-shark/src/libkshark-collection.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)