* [PATCH 0/4] [media] bt8xx: Fine-tuning for three functions @ 2016-12-10 20:45 SF Markus Elfring 2016-12-10 20:48 ` [PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection SF Markus Elfring ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: SF Markus Elfring @ 2016-12-10 20:45 UTC (permalink / raw) To: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab Cc: LKML, kernel-janitors From: Markus Elfring <elfring@users.sourceforge.net> Date: Sat, 10 Dec 2016 21:40:12 +0100 A few update suggestions were taken into account from static source code analysis. Markus Elfring (4): One function call less in bttv_input_init() after error detection Delete two error messages for a failed memory allocation Delete unnecessary variable initialisations in ca_send_message() Less function calls in dst_ca_ioctl() after error detection drivers/media/pci/bt8xx/bttv-input.c | 14 +++++--- drivers/media/pci/bt8xx/dst_ca.c | 62 +++++++++++++++++++++--------------- 2 files changed, 46 insertions(+), 30 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection 2016-12-10 20:45 [PATCH 0/4] [media] bt8xx: Fine-tuning for three functions SF Markus Elfring @ 2016-12-10 20:48 ` SF Markus Elfring 2016-12-10 21:29 ` Daniele Nicolodi 2016-12-10 20:50 ` [PATCH 2/4] [media] bt8xx: Delete two error messages for a failed memory allocation SF Markus Elfring ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: SF Markus Elfring @ 2016-12-10 20:48 UTC (permalink / raw) To: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab Cc: LKML, kernel-janitors From: Markus Elfring <elfring@users.sourceforge.net> Date: Sat, 10 Dec 2016 09:29:24 +0100 The kfree() function was called in one case by the bttv_input_init() function during error handling even if the passed variable contained a null pointer. This issue was detected by using the Coccinelle software. * Split a condition check for resource allocation failures so that each pointer from these function calls will be checked immediately. See also background information: Topic "CWE-754: Improper check for unusual or exceptional conditions" Link: https://cwe.mitre.org/data/definitions/754.html Fixes: d8b4b5822f51e2142b731b42c81e3f03eec475b2 ("[media] ir-core: make struct rc_dev the primary interface") * Adjust a jump target according to the Linux coding style convention. * Delete an initialisation for the variable "err" at the beginning which became unnecessary with this refactoring. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- drivers/media/pci/bt8xx/bttv-input.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/media/pci/bt8xx/bttv-input.c b/drivers/media/pci/bt8xx/bttv-input.c index 4da720e4867e..9187993d23ea 100644 --- a/drivers/media/pci/bt8xx/bttv-input.c +++ b/drivers/media/pci/bt8xx/bttv-input.c @@ -418,15 +418,20 @@ int bttv_input_init(struct bttv *btv) struct bttv_ir *ir; char *ir_codes = NULL; struct rc_dev *rc; - int err = -ENOMEM; + int err; if (!btv->has_remote) return -ENODEV; - ir = kzalloc(sizeof(*ir),GFP_KERNEL); + ir = kzalloc(sizeof(*ir), GFP_KERNEL); + if (!ir) + return -ENOMEM; + rc = rc_allocate_device(); - if (!ir || !rc) - goto err_out_free; + if (!rc) { + err = -ENOMEM; + goto free_ir; + } /* detect & configure */ switch (btv->c.type) { @@ -569,6 +574,7 @@ int bttv_input_init(struct bttv *btv) btv->remote = NULL; err_out_free: rc_free_device(rc); +free_ir: kfree(ir); return err; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection 2016-12-10 20:48 ` [PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection SF Markus Elfring @ 2016-12-10 21:29 ` Daniele Nicolodi 2016-12-10 22:10 ` SF Markus Elfring 0 siblings, 1 reply; 17+ messages in thread From: Daniele Nicolodi @ 2016-12-10 21:29 UTC (permalink / raw) To: SF Markus Elfring, linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab Cc: LKML, kernel-janitors On 10/12/16 13:48, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sat, 10 Dec 2016 09:29:24 +0100 > > The kfree() function was called in one case by the > bttv_input_init() function during error handling > even if the passed variable contained a null pointer. kfree() is safe to call on a NULL pointer. Despite that, you have found several instances of similar constructs: { a = kmalloc(...) b = kmalloc(...) if (!a || !b) goto out; ... out: kfree(a); kfree(b); } and similar patches you submitted to change those construct to something different have been rejected because they are seen as unnecessary changes that make the code harder to read. Didn't it occur to you that maybe those constructs are fine the way they are and this is the idiomatic way to write that kind of code? Why are you submitting patches implementing changes that have already been rejected? Submitting mechanical patches that fix trivial style issues (existing and widely acknowledged ones) is a fine way to learn how to work on kernel development. They constitute additional work load on the maintainers that need to review and merge them. Thus, hopefully, they are only a way for new developers to familiarize themselves with the process and then move to some more constructive contributions. Judging from your recent submissions, it seems that this process is not working well for you. I'm probably not the only one that is wonderign what are you trying to obtain with your patch submissions, other than having your name in the git log. Cheers, Daniele ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [media] bt8xx: One function call less in bttv_input_init() after error detection 2016-12-10 21:29 ` Daniele Nicolodi @ 2016-12-10 22:10 ` SF Markus Elfring 2016-12-11 21:52 ` Daniele Nicolodi 0 siblings, 1 reply; 17+ messages in thread From: SF Markus Elfring @ 2016-12-10 22:10 UTC (permalink / raw) To: Daniele Nicolodi Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors > kfree() is safe to call on a NULL pointer. This is true. > Despite that, you have found several instances of similar constructs: Yes. - Special source code search pattern can point such places out for further considerations. > Didn't it occur to you that maybe those constructs are fine the way > they are and this is the idiomatic way to write that kind of code? Such a programming approach might look convenient. - I would prefer a safer coding style for the corresponding exception handling. > Why are you submitting patches implementing changes that have already > been rejected? The feedback to my update mixture is varying between acceptance and disagreements as usual. > Judging from your recent submissions, it seems that this process is not > working well for you. I'm probably not the only one that is wonderign > what are you trying to obtain with your patch submissions, other than > having your name in the git log. I am picking some change possibilities up in the hope of related software improvements. Regards, Markus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [media] bt8xx: One function call less in bttv_input_init() after error detection 2016-12-10 22:10 ` SF Markus Elfring @ 2016-12-11 21:52 ` Daniele Nicolodi 2016-12-12 7:33 ` SF Markus Elfring 0 siblings, 1 reply; 17+ messages in thread From: Daniele Nicolodi @ 2016-12-11 21:52 UTC (permalink / raw) To: SF Markus Elfring Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors On 10/12/16 15:10, SF Markus Elfring wrote: >> Despite that, you have found several instances of similar constructs: > > Yes. - Special source code search pattern can point such places out > for further considerations. This is one of the things that makes reviewing the patches you submit quire annoying: if a class of changes has already been turned town, why do you insist in proposing it? >> Didn't it occur to you that maybe those constructs are fine the way >> they are and this is the idiomatic way to write that kind of code? > > Such a programming approach might look convenient. - I would prefer > a safer coding style for the corresponding exception handling. Can you please point out what is wrong in the current code and how the changes you propose fix the problem? Clearly the people reading your patches do not see the problem you are seeing. >> Why are you submitting patches implementing changes that have already >> been rejected? > > The feedback to my update mixture is varying between acceptance and > disagreements as usual. No one has expressed acceptance for the kind of change you propose with this patch, or to previous patches you proposed changing similar constructs. >> Judging from your recent submissions, it seems that this process is not >> working well for you. I'm probably not the only one that is wonderign >> what are you trying to obtain with your patch submissions, other than >> having your name in the git log. > > I am picking some change possibilities up in the hope of related > software improvements. The fact that you propose over and over again a class of changes that has been already vocally rejected would suggest otherwise. The major achievement you obtained so far is that one of the maintainers of a large fraction of the kernel refuses to look at your patch submissions. Maybe you should revise how you contribute to Linux kernel development. Apparently ignoring comments to your previous patch submission and not showing that you have been learning from previous interactions, is not going to help. Cheers, Daniele ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [media] bt8xx: One function call less in bttv_input_init() after error detection 2016-12-11 21:52 ` Daniele Nicolodi @ 2016-12-12 7:33 ` SF Markus Elfring 2016-12-12 7:39 ` Daniele Nicolodi 0 siblings, 1 reply; 17+ messages in thread From: SF Markus Elfring @ 2016-12-12 7:33 UTC (permalink / raw) To: Daniele Nicolodi Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors >> I would prefer a safer coding style for the corresponding >> exception handling. > > Can you please point out what is wrong in the current code Is it useful to reconsider the software situation that another memory allocation is attempted when it could be determined that a previous one failed already? Are two successful allocations finally needed to achieve the desired task? > and how the changes you propose fix the problem? I suggest to check return values immediately after each function call. An error situation can be detected earlier then and only the required clean-up functionality will be executed at the end. > No one has expressed acceptance for the kind of change you propose with > this patch, or to previous patches you proposed changing similar constructs. I got a mixed impression from the acceptance statistics about my published patches. > The fact that you propose over and over again a class of changes that > has been already vocally rejected would suggest otherwise. I dare to propose another look at results from source code search patterns. > The major achievement you obtained so far is that one of the maintainers > of a large fraction of the kernel refuses to look at your patch submissions. It can happen that some patterns are occasionally "too special" to grow the popularity for such change possibilities and desired software improvements quickly. There are also different views about affected implementation details by the software development community, aren't there? Regards, Markus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [media] bt8xx: One function call less in bttv_input_init() after error detection 2016-12-12 7:33 ` SF Markus Elfring @ 2016-12-12 7:39 ` Daniele Nicolodi 2016-12-12 17:15 ` SF Markus Elfring ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Daniele Nicolodi @ 2016-12-12 7:39 UTC (permalink / raw) To: SF Markus Elfring Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors On 12/12/16 00:33, SF Markus Elfring wrote: >>> I would prefer a safer coding style for the corresponding >>> exception handling. >> >> Can you please point out what is wrong in the current code > > Is it useful to reconsider the software situation that another memory > allocation is attempted when it could be determined that a previous one > failed already? No. > Are two successful allocations finally needed to achieve the desired task? Yes. >> and how the changes you propose fix the problem? > > I suggest to check return values immediately after each function call. > An error situation can be detected earlier then and only the required > clean-up functionality will be executed at the end. Which improvement does this bring? >> No one has expressed acceptance for the kind of change you propose with >> this patch, or to previous patches you proposed changing similar constructs. > > I got a mixed impression from the acceptance statistics about my > published patches. Have you proposed a similar patch that was accepted? I don't find record of it, but I may be wrong. >> The fact that you propose over and over again a class of changes that >> has been already vocally rejected would suggest otherwise. > > I dare to propose another look at results from source code search patterns. Why? Cheers, Daniele ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [media] bt8xx: One function call less in bttv_input_init() after error detection 2016-12-12 7:39 ` Daniele Nicolodi @ 2016-12-12 17:15 ` SF Markus Elfring 2016-12-12 17:56 ` Daniele Nicolodi 2016-12-12 18:03 ` Clarification for acceptance statistics? SF Markus Elfring 2016-12-12 19:11 ` [media] bt8xx: One function call less in bttv_input_init() after error detection Dan Carpenter 2 siblings, 1 reply; 17+ messages in thread From: SF Markus Elfring @ 2016-12-12 17:15 UTC (permalink / raw) To: Daniele Nicolodi Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors >> I suggest to check return values immediately after each function call. >> An error situation can be detected earlier then and only the required >> clean-up functionality will be executed at the end. > > Which improvement does this bring? * How do you think about to avoid requesting additional system resources when it was determined that a previously required memory allocation failed? * Can the corresponding exception handling become a bit more efficient? > Why? Do you care for any results from static source code analysis? Regards, Markus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [media] bt8xx: One function call less in bttv_input_init() after error detection 2016-12-12 17:15 ` SF Markus Elfring @ 2016-12-12 17:56 ` Daniele Nicolodi 0 siblings, 0 replies; 17+ messages in thread From: Daniele Nicolodi @ 2016-12-12 17:56 UTC (permalink / raw) To: SF Markus Elfring Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors On 12/12/16 10:15 AM, SF Markus Elfring wrote: >>> I suggest to check return values immediately after each function call. >>> An error situation can be detected earlier then and only the required >>> clean-up functionality will be executed at the end. >> >> Which improvement does this bring? > > * How do you think about to avoid requesting additional system resources > when it was determined that a previously required memory allocation failed? I think it is an irrelevant problem in the case at hand. > * Can the corresponding exception handling become a bit more efficient? Where more efficient merely means sparing one function call? I think it is completely irrelevant in the case at hand and code clarity must be preferred to pointless optimization. >> Why? > > Do you care for any results from static source code analysis? Static source code analysis, in the form you are doing with Coccinelle, may help in identifying problems in a code base when a specific pattern has been identified to be problematic. In the static code analysis results you present, it is not clear what the problematic pattern is. Independently of how you identified the code section you propose to change, there is no problem to fix. As a general advise, Markus, replying to questions with other questions is a a bad debate form. Questions, as opposed to statements, cannot be confuted. Also, every time you receive an answer to one of your questions, you reply with another question broadening the span of the discussion. However, you do not present evidence supporting your initial statement that some changes in the code are beneficial. Cheers, Daniele ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Clarification for acceptance statistics? 2016-12-12 7:39 ` Daniele Nicolodi 2016-12-12 17:15 ` SF Markus Elfring @ 2016-12-12 18:03 ` SF Markus Elfring 2016-12-12 21:02 ` Daniele Nicolodi 2016-12-12 19:11 ` [media] bt8xx: One function call less in bttv_input_init() after error detection Dan Carpenter 2 siblings, 1 reply; 17+ messages in thread From: SF Markus Elfring @ 2016-12-12 18:03 UTC (permalink / raw) To: Daniele Nicolodi Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors > Have you proposed a similar patch that was accepted? Yes. - It happened a few times. > I don't find record of it, but I may be wrong. It is really needed to clarify the corresponding software development history any further? Regards, Markus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Clarification for acceptance statistics? 2016-12-12 18:03 ` Clarification for acceptance statistics? SF Markus Elfring @ 2016-12-12 21:02 ` Daniele Nicolodi 2016-12-12 22:11 ` SF Markus Elfring 0 siblings, 1 reply; 17+ messages in thread From: Daniele Nicolodi @ 2016-12-12 21:02 UTC (permalink / raw) To: SF Markus Elfring Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors On 12/12/16 11:03 AM, SF Markus Elfring wrote: >> Have you proposed a similar patch that was accepted? > > Yes. - It happened a few times. The question was: have you ever had a patch changing code in the form { a = kmalloc(...); b = kmalloc(...); if (!a || !b) goto out; ... out: kfree(a); kfree(b); } to something else, accepted? I went checking and I haven't found such a patch. Did you understand my question? > It is really needed to clarify the corresponding software development > history any further? It is relevant because you are submitting a patch and your changelog implies that it makes the code follow some code structure rule that needs to be applied to the kernel. As the above is a recurring pattern in kernel code, it is legitimate to ask if such a rule exist, and has been enforced before, or you are making it up. My conclusion is that you are making it up. As a proposer of a new pattern, what is the evidence you can bring to the discussion that supports that your solution is better? What is the metric you are using to define "better"? Cheers, Daniele ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Clarification for acceptance statistics? 2016-12-12 21:02 ` Daniele Nicolodi @ 2016-12-12 22:11 ` SF Markus Elfring 2016-12-12 23:19 ` Daniele Nicolodi 0 siblings, 1 reply; 17+ messages in thread From: SF Markus Elfring @ 2016-12-12 22:11 UTC (permalink / raw) To: Daniele Nicolodi Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors > The question was: have you ever had a patch changing code in the form > > { > a = kmalloc(...); > b = kmalloc(...); > > if (!a || !b) > goto out; > > ... > > out: > kfree(a); > kfree(b); > } > > to something else, accepted? It seems that this case did not happen for me so far if you are looking for this exact source code search pattern. Variants of the current pattern were occasionally discussed a bit. > I went checking and I haven't found such a patch. A few similar update suggestions are still in development waiting queues. > Did you understand my question? Partly. - My interpretation of similar changes was eventually too broad in my previous answer. >> It is really needed to clarify the corresponding software development >> history any further? > > It is relevant because you are submitting a patch and your changelog > implies that it makes the code follow some code structure rule that > needs to be applied to the kernel. I am proposing a change which was described also around various other functions in some software already. > As the above is a recurring pattern in kernel code, it is legitimate > to ask if such a rule exist, and has been enforced before, or you are > making it up. I got the impression that special software development habits can also evolve over time. > As a proposer of a new pattern, what is the evidence you can bring to > the discussion that supports that your solution is better? I am trying to increase the software development attention on error detection and corresponding exception handling at various places. > What is the metric you are using to define "better"? Do response times for system failures matter here? Regards, Markus ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Clarification for acceptance statistics? 2016-12-12 22:11 ` SF Markus Elfring @ 2016-12-12 23:19 ` Daniele Nicolodi 0 siblings, 0 replies; 17+ messages in thread From: Daniele Nicolodi @ 2016-12-12 23:19 UTC (permalink / raw) To: SF Markus Elfring Cc: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors On 12/12/16 3:11 PM, SF Markus Elfring wrote: >>> It is really needed to clarify the corresponding software development >>> history any further? >> >> It is relevant because you are submitting a patch and your changelog >> implies that it makes the code follow some code structure rule that >> needs to be applied to the kernel. > > I am proposing a change which was described also around various other > functions in some software already. What is this supposed to mean? >> As the above is a recurring pattern in kernel code, it is legitimate >> to ask if such a rule exist, and has been enforced before, or you are >> making it up. > > I got the impression that special software development habits can also > evolve over time. > >> As a proposer of a new pattern, what is the evidence you can bring to >> the discussion that supports that your solution is better? > > I am trying to increase the software development attention on error > detection and corresponding exception handling at various places. Are you doing this submitting random patches to the kernel sources? >> What is the metric you are using to define "better"? > > Do response times for system failures matter here? No. And you are again answering a question with a question. Cheers, Daniele ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [media] bt8xx: One function call less in bttv_input_init() after error detection 2016-12-12 7:39 ` Daniele Nicolodi 2016-12-12 17:15 ` SF Markus Elfring 2016-12-12 18:03 ` Clarification for acceptance statistics? SF Markus Elfring @ 2016-12-12 19:11 ` Dan Carpenter 2 siblings, 0 replies; 17+ messages in thread From: Dan Carpenter @ 2016-12-12 19:11 UTC (permalink / raw) To: Daniele Nicolodi Cc: SF Markus Elfring, linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab, LKML, kernel-janitors You will never win an ELIZA bot challenge against Markus. :P I admire your optimism though. regards, dan carpenter ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/4] [media] bt8xx: Delete two error messages for a failed memory allocation 2016-12-10 20:45 [PATCH 0/4] [media] bt8xx: Fine-tuning for three functions SF Markus Elfring 2016-12-10 20:48 ` [PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection SF Markus Elfring @ 2016-12-10 20:50 ` SF Markus Elfring 2016-12-10 20:51 ` [PATCH 3/4] [media] bt8xx: Delete unnecessary variable initialisations in ca_send_message() SF Markus Elfring 2016-12-10 20:53 ` [PATCH 4/4] [media] bt8xx: Less function calls in dst_ca_ioctl() after error detection SF Markus Elfring 3 siblings, 0 replies; 17+ messages in thread From: SF Markus Elfring @ 2016-12-10 20:50 UTC (permalink / raw) To: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab Cc: LKML, kernel-janitors, Wolfram Sang From: Markus Elfring <elfring@users.sourceforge.net> Date: Sat, 10 Dec 2016 20:50:58 +0100 Omit extra messages for a memory allocation failure in two functions. Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- drivers/media/pci/bt8xx/dst_ca.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c index 8681b9143a35..54e656ddd588 100644 --- a/drivers/media/pci/bt8xx/dst_ca.c +++ b/drivers/media/pci/bt8xx/dst_ca.c @@ -481,10 +481,9 @@ static int ca_send_message(struct dst_state *state, struct ca_msg *p_ca_message, struct ca_msg *hw_buffer; int result = 0; - if ((hw_buffer = kmalloc(sizeof (struct ca_msg), GFP_KERNEL)) == NULL) { - dprintk(verbose, DST_CA_ERROR, 1, " Memory allocation failure"); + hw_buffer = kmalloc(sizeof(*hw_buffer), GFP_KERNEL); + if (!hw_buffer) return -ENOMEM; - } dprintk(verbose, DST_CA_DEBUG, 1, " "); if (copy_from_user(p_ca_message, arg, sizeof (struct ca_msg))) { @@ -567,7 +566,6 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct p_ca_slot_info = kmalloc(sizeof (struct ca_slot_info), GFP_KERNEL); p_ca_caps = kmalloc(sizeof (struct ca_caps), GFP_KERNEL); if (!p_ca_message || !p_ca_slot_info || !p_ca_caps) { - dprintk(verbose, DST_CA_ERROR, 1, " Memory allocation failure"); result = -ENOMEM; goto free_mem_and_exit; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/4] [media] bt8xx: Delete unnecessary variable initialisations in ca_send_message() 2016-12-10 20:45 [PATCH 0/4] [media] bt8xx: Fine-tuning for three functions SF Markus Elfring 2016-12-10 20:48 ` [PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection SF Markus Elfring 2016-12-10 20:50 ` [PATCH 2/4] [media] bt8xx: Delete two error messages for a failed memory allocation SF Markus Elfring @ 2016-12-10 20:51 ` SF Markus Elfring 2016-12-10 20:53 ` [PATCH 4/4] [media] bt8xx: Less function calls in dst_ca_ioctl() after error detection SF Markus Elfring 3 siblings, 0 replies; 17+ messages in thread From: SF Markus Elfring @ 2016-12-10 20:51 UTC (permalink / raw) To: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab Cc: LKML, kernel-janitors From: Markus Elfring <elfring@users.sourceforge.net> Date: Sat, 10 Dec 2016 20:56:04 +0100 Two local variables will be set to an appropriate value a bit later. Thus omit the explicit initialisation at the beginning. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- drivers/media/pci/bt8xx/dst_ca.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c index 54e656ddd588..04d06c564602 100644 --- a/drivers/media/pci/bt8xx/dst_ca.c +++ b/drivers/media/pci/bt8xx/dst_ca.c @@ -475,9 +475,8 @@ static int dst_check_ca_pmt(struct dst_state *state, struct ca_msg *p_ca_message static int ca_send_message(struct dst_state *state, struct ca_msg *p_ca_message, void __user *arg) { - int i = 0; - - u32 command = 0; + int i; + u32 command; struct ca_msg *hw_buffer; int result = 0; -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/4] [media] bt8xx: Less function calls in dst_ca_ioctl() after error detection 2016-12-10 20:45 [PATCH 0/4] [media] bt8xx: Fine-tuning for three functions SF Markus Elfring ` (2 preceding siblings ...) 2016-12-10 20:51 ` [PATCH 3/4] [media] bt8xx: Delete unnecessary variable initialisations in ca_send_message() SF Markus Elfring @ 2016-12-10 20:53 ` SF Markus Elfring 3 siblings, 0 replies; 17+ messages in thread From: SF Markus Elfring @ 2016-12-10 20:53 UTC (permalink / raw) To: linux-media, Alexey Khoroshilov, Hans Verkuil, Mauro Carvalho Chehab Cc: LKML, kernel-janitors From: Markus Elfring <elfring@users.sourceforge.net> Date: Sat, 10 Dec 2016 21:30:10 +0100 The kfree() function was called in up to three cases by the dst_ca_ioctl() function during error handling even if the passed variable contained a null pointer. This issue was detected by using the Coccinelle software. * Split a condition check for memory allocation failures so that each pointer from these function calls will be checked immediately. See also background information: Topic "CWE-754: Improper check for unusual or exceptional conditions" Link: https://cwe.mitre.org/data/definitions/754.html Fixes: b57e5578f913a304e97cb66aa0044a894ca47f2f ("Fixes some sync issues between V4L/DVB development and GIT") * Replace the specification of data structures by pointer dereferences to make the corresponding size determination a bit safer. * Adjust jump targets according to the Linux coding style convention. Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> --- drivers/media/pci/bt8xx/dst_ca.c | 51 +++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c index 04d06c564602..50cdb53c9e8a 100644 --- a/drivers/media/pci/bt8xx/dst_ca.c +++ b/drivers/media/pci/bt8xx/dst_ca.c @@ -559,16 +559,27 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct int result = 0; mutex_lock(&dst_ca_mutex); - dvbdev = file->private_data; - state = (struct dst_state *)dvbdev->priv; - p_ca_message = kmalloc(sizeof (struct ca_msg), GFP_KERNEL); - p_ca_slot_info = kmalloc(sizeof (struct ca_slot_info), GFP_KERNEL); - p_ca_caps = kmalloc(sizeof (struct ca_caps), GFP_KERNEL); - if (!p_ca_message || !p_ca_slot_info || !p_ca_caps) { + p_ca_message = kmalloc(sizeof(*p_ca_message), GFP_KERNEL); + if (!p_ca_message) { result = -ENOMEM; - goto free_mem_and_exit; + goto unlock; + } + + p_ca_slot_info = kmalloc(sizeof(*p_ca_slot_info), GFP_KERNEL); + if (!p_ca_slot_info) { + result = -ENOMEM; + goto free_message; } + p_ca_caps = kmalloc(sizeof(*p_ca_caps), GFP_KERNEL); + if (!p_ca_caps) { + result = -ENOMEM; + goto free_slot_info; + } + + dvbdev = file->private_data; + state = (struct dst_state *)dvbdev->priv; + /* We have now only the standard ioctl's, the driver is upposed to handle internals. */ switch (cmd) { case CA_SEND_MSG: @@ -576,7 +587,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct if ((ca_send_message(state, p_ca_message, arg)) < 0) { dprintk(verbose, DST_CA_ERROR, 1, " -->CA_SEND_MSG Failed !"); result = -1; - goto free_mem_and_exit; + goto free_caps; } break; case CA_GET_MSG: @@ -584,7 +595,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct if ((ca_get_message(state, p_ca_message, arg)) < 0) { dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_MSG Failed !"); result = -1; - goto free_mem_and_exit; + goto free_caps; } dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_MSG Success !"); break; @@ -598,7 +609,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct if ((ca_get_slot_info(state, p_ca_slot_info, arg)) < 0) { dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_SLOT_INFO Failed !"); result = -1; - goto free_mem_and_exit; + goto free_caps; } dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_SLOT_INFO Success !"); break; @@ -607,7 +618,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct if ((ca_get_slot_caps(state, p_ca_caps, arg)) < 0) { dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_CAP Failed !"); result = -1; - goto free_mem_and_exit; + goto free_caps; } dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_CAP Success !"); break; @@ -616,7 +627,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct if ((ca_get_slot_descr(state, p_ca_message, arg)) < 0) { dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_DESCR_INFO Failed !"); result = -1; - goto free_mem_and_exit; + goto free_caps; } dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_DESCR_INFO Success !"); break; @@ -625,7 +636,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct if ((ca_set_slot_descr()) < 0) { dprintk(verbose, DST_CA_ERROR, 1, " -->CA_SET_DESCR Failed !"); result = -1; - goto free_mem_and_exit; + goto free_caps; } dprintk(verbose, DST_CA_INFO, 1, " -->CA_SET_DESCR Success !"); break; @@ -634,17 +645,19 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct if ((ca_set_pid()) < 0) { dprintk(verbose, DST_CA_ERROR, 1, " -->CA_SET_PID Failed !"); result = -1; - goto free_mem_and_exit; + goto free_caps; } dprintk(verbose, DST_CA_INFO, 1, " -->CA_SET_PID Success !"); default: result = -EOPNOTSUPP; } - free_mem_and_exit: - kfree (p_ca_message); - kfree (p_ca_slot_info); - kfree (p_ca_caps); - +free_caps: + kfree(p_ca_caps); +free_slot_info: + kfree(p_ca_slot_info); +free_message: + kfree(p_ca_message); +unlock: mutex_unlock(&dst_ca_mutex); return result; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-12-12 23:19 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-10 20:45 [PATCH 0/4] [media] bt8xx: Fine-tuning for three functions SF Markus Elfring 2016-12-10 20:48 ` [PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection SF Markus Elfring 2016-12-10 21:29 ` Daniele Nicolodi 2016-12-10 22:10 ` SF Markus Elfring 2016-12-11 21:52 ` Daniele Nicolodi 2016-12-12 7:33 ` SF Markus Elfring 2016-12-12 7:39 ` Daniele Nicolodi 2016-12-12 17:15 ` SF Markus Elfring 2016-12-12 17:56 ` Daniele Nicolodi 2016-12-12 18:03 ` Clarification for acceptance statistics? SF Markus Elfring 2016-12-12 21:02 ` Daniele Nicolodi 2016-12-12 22:11 ` SF Markus Elfring 2016-12-12 23:19 ` Daniele Nicolodi 2016-12-12 19:11 ` [media] bt8xx: One function call less in bttv_input_init() after error detection Dan Carpenter 2016-12-10 20:50 ` [PATCH 2/4] [media] bt8xx: Delete two error messages for a failed memory allocation SF Markus Elfring 2016-12-10 20:51 ` [PATCH 3/4] [media] bt8xx: Delete unnecessary variable initialisations in ca_send_message() SF Markus Elfring 2016-12-10 20:53 ` [PATCH 4/4] [media] bt8xx: Less function calls in dst_ca_ioctl() after error detection SF Markus Elfring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).