| Message ID | 20190717085306.12393-2-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for KS 1.0 | expand |
On Wed, 17 Jul 2019 11:53:05 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > It is particularly important to initialize to zero the "data_size" field > because its value is used when doing operations like scroll or zoom to > check if data has been loaded or not. Not having "data_size" set to zero > can cause segfault (as reported by Steven). > > Reported-By: Steven Rostedt (VMware) <rostedt@goodmis.org> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195 > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > kernel-shark/src/libkshark-model.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c > index 18f9c69..fd4d876 100644 > --- a/kernel-shark/src/libkshark-model.c > +++ b/kernel-shark/src/libkshark-model.c > @@ -36,6 +36,9 @@ void ksmodel_init(struct kshark_trace_histo *histo) > * Initialize an empty histo. The histo will have no bins and will > * contain no data. > */ > + histo->data_size = 0; > + histo->data = NULL; > + > histo->bin_size = 0; > histo->min = 0; > histo->max = 0; Are we just trying to set all fields of histo to NULL or zero? If so, why not just do: memset(histo, 0, sizeof(*histo)); ? This will make sure ksmodel_init() zeros all of histo when/if we add new fields. -- Steve
On 17.07.19 г. 15:37 ч., Steven Rostedt wrote: > On Wed, 17 Jul 2019 11:53:05 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >> It is particularly important to initialize to zero the "data_size" field >> because its value is used when doing operations like scroll or zoom to >> check if data has been loaded or not. Not having "data_size" set to zero >> can cause segfault (as reported by Steven). >> >> Reported-By: Steven Rostedt (VMware) <rostedt@goodmis.org> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195 >> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> >> --- >> kernel-shark/src/libkshark-model.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c >> index 18f9c69..fd4d876 100644 >> --- a/kernel-shark/src/libkshark-model.c >> +++ b/kernel-shark/src/libkshark-model.c >> @@ -36,6 +36,9 @@ void ksmodel_init(struct kshark_trace_histo *histo) >> * Initialize an empty histo. The histo will have no bins and will >> * contain no data. >> */ >> + histo->data_size = 0; >> + histo->data = NULL; >> + >> histo->bin_size = 0; >> histo->min = 0; >> histo->max = 0; > > Are we just trying to set all fields of histo to NULL or zero? If so, > why not just do: > > memset(histo, 0, sizeof(*histo)); > > ? > > This will make sure ksmodel_init() zeros all of histo when/if we add new > fields. > Yes, this will do a better job. Thanks! Yordan > -- Steve >
On Wed, 17 Jul 2019 15:42:31 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > > Are we just trying to set all fields of histo to NULL or zero? If so, > > why not just do: > > > > memset(histo, 0, sizeof(*histo)); > > > > ? > > > > This will make sure ksmodel_init() zeros all of histo when/if we add new > > fields. > > > > Yes, this will do a better job. Care to send a v2 patch? Thanks Yordan! -- Steve
On 17.07.19 г. 16:28 ч., Steven Rostedt wrote: > On Wed, 17 Jul 2019 15:42:31 +0300 > "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > >>> Are we just trying to set all fields of histo to NULL or zero? If so, >>> why not just do: >>> >>> memset(histo, 0, sizeof(*histo)); >>> >>> ? >>> >>> This will make sure ksmodel_init() zeros all of histo when/if we add new >>> fields. >>> >> >> Yes, this will do a better job. > > Care to send a v2 patch? Just make the change and apply, if this is OK with you. Thanks a lot! Yordan > > Thanks Yordan! > > -- Steve >
On Wed, 17 Jul 2019 16:38:47 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > > > > Care to send a v2 patch? > > Just make the change and apply, if this is OK with you. > Here's the new patch: From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Subject: [PATCH] kernel-shark: Initialize all fields of struct kshark_trace_histo The function ksmodel_init() is to initialize the kshark_trace_histo structure to zero. Currently it does it via each field. It is safer to use memset() that will guarantee that the entire structure is set to zeros or NULLs if new fields are added. This is required because there's places in the code that check if a field is NULL or zero to determine if it should be set or not. Link: http://lkml.kernel.org/r/20190717085306.12393-2-y.karadz@gmail.com Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195 Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel-shark/src/libkshark-model.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c index 18f9c691..6c54e1e1 100644 --- a/kernel-shark/src/libkshark-model.c +++ b/kernel-shark/src/libkshark-model.c @@ -36,13 +36,7 @@ void ksmodel_init(struct kshark_trace_histo *histo) * Initialize an empty histo. The histo will have no bins and will * contain no data. */ - histo->bin_size = 0; - histo->min = 0; - histo->max = 0; - histo->n_bins = 0; - - histo->bin_count = NULL; - histo->map = NULL; + memset(histo, 0, sizeof(*histo)); } /**
And here's the version that claws-mail didn't line wrap :-p -- Steve From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Subject: [PATCH] kernel-shark: Initialize all fields of struct kshark_trace_histo The function ksmodel_init() is to initialize the kshark_trace_histo structure to zero. Currently it does it via each field. It is safer to use memset() that will guarantee that the entire structure is set to zeros or NULLs if new fields are added. This is required because there's places in the code that check if a field is NULL or zero to determine if it should be set or not. Link: http://lkml.kernel.org/r/20190717085306.12393-2-y.karadz@gmail.com Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195 Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel-shark/src/libkshark-model.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c index 18f9c691..6c54e1e1 100644 --- a/kernel-shark/src/libkshark-model.c +++ b/kernel-shark/src/libkshark-model.c @@ -36,13 +36,7 @@ void ksmodel_init(struct kshark_trace_histo *histo) * Initialize an empty histo. The histo will have no bins and will * contain no data. */ - histo->bin_size = 0; - histo->min = 0; - histo->max = 0; - histo->n_bins = 0; - - histo->bin_count = NULL; - histo->map = NULL; + memset(histo, 0, sizeof(*histo)); } /**
On 17.07.19 г. 22:27 ч., Steven Rostedt wrote: > > And here's the version that claws-mail didn't line wrap :-p > > -- Steve > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > Subject: [PATCH] kernel-shark: Initialize all fields of struct kshark_trace_histo > > The function ksmodel_init() is to initialize the kshark_trace_histo > structure to zero. Currently it does it via each field. It is safer to use > memset() that will guarantee that the entire structure is set to zeros or > NULLs if new fields are added. This is required because there's places in > the code that check if a field is NULL or zero to determine if it should be > set or not. > > Link: http://lkml.kernel.org/r/20190717085306.12393-2-y.karadz@gmail.com > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195 > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel-shark/src/libkshark-model.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c > index 18f9c691..6c54e1e1 100644 > --- a/kernel-shark/src/libkshark-model.c > +++ b/kernel-shark/src/libkshark-model.c > @@ -36,13 +36,7 @@ void ksmodel_init(struct kshark_trace_histo *histo) > * Initialize an empty histo. The histo will have no bins and will > * contain no data. > */ > - histo->bin_size = 0; > - histo->min = 0; > - histo->max = 0; > - histo->n_bins = 0; > - > - histo->bin_count = NULL; > - histo->map = NULL; > + memset(histo, 0, sizeof(*histo)); > } > > /** > Thanks! Yordan Reviewed-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c index 18f9c69..fd4d876 100644 --- a/kernel-shark/src/libkshark-model.c +++ b/kernel-shark/src/libkshark-model.c @@ -36,6 +36,9 @@ void ksmodel_init(struct kshark_trace_histo *histo) * Initialize an empty histo. The histo will have no bins and will * contain no data. */ + histo->data_size = 0; + histo->data = NULL; + histo->bin_size = 0; histo->min = 0; histo->max = 0;
It is particularly important to initialize to zero the "data_size" field because its value is used when doing operations like scroll or zoom to check if data has been loaded or not. Not having "data_size" set to zero can cause segfault (as reported by Steven). Reported-By: Steven Rostedt (VMware) <rostedt@goodmis.org> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195 Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- kernel-shark/src/libkshark-model.c | 3 +++ 1 file changed, 3 insertions(+)