random fixes
Subsystems (10)
cover random fixes
next round of random fixes reported by codacy:
https://app.codacy.com/app/olerem/barebox/issues/index?bid=9803255
Oleksij Rempel (9):
net: e1000: fix "Uninitialized variable: phy_data" warning
pinctrl: tegra30: fix "Possible null pointer dereference: group"
warning
fs: closedir: remove uninitialized variable ret
fs: fix possible null pointer dereference of base.
mfd: da9063: fix wrong NULL pointer test
usb: at91_udc: remove useless NULL check
mtd: atmel_nand: remove erroneous case
usb: musb: fix possible out of bounds access
lib: lodepng: remove useless test
drivers/mfd/da9063.c | 2 +- drivers/mtd/nand/atmel_nand.c | 1 - drivers/net/e1000/main.c | 2 +- drivers/pinctrl/pinctrl-tegra30.c | 4 ++-- drivers/usb/gadget/at91_udc.c | 3 +-- drivers/usb/musb/musb_gadget_ep0.c | 7 ++++++- fs/fs.c | 7 +++---- lib/gui/lodepng.c | 8 +------- 8 files changed, 15 insertions(+), 19 deletions(-)
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 1/9 net: e1000: fix "Uninitialized variable: phy_data" warning
After carefully reading the code, this situation should never happen. This patch is to reduce warning noise.
--- drivers/net/e1000/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/e1000/main.c +1 -1
@@ -1130,7 +1130,7 @@ static int32_t e1000_set_d3_lplu_state_off(struct e1000_hw *hw) {uint32_t phy_ctrl =0 ;int32_t ret_val; -uint16_t phy_data; +uint16_t phy_data =0 ;DEBUGFUNC ();/* During driver activity LPLU should not be used or it will attain link
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 2/9 pinctrl: tegra30: fix "Possible null pointer dereference: group" warning
In reply to Oleksij RempelThe code is correct but it takes more seconds for me to understand. And static code analyzer do not understand it at all.
--- drivers/pinctrl/pinctrl-tegra30.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pinctrl/pinctrl-tegra30.c +2 -2
@@ -658,8 +658,8 @@ static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl,break ; } } -/* if no matching drivegroup is found */ -if (i == ctrl->drvdata->num_drvgrps) + +if (!group)return 0 ; regaddr = ctrl->regs.ctrl + (group->reg >>2 );
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v1 2/9] pinctrl: tegra30: fix "Possible null pointer dereference: group" warning
Am Dienstag, den 20.11.2018, 21:07 +0100 schrieb Oleksij Rempel:
Quoted 19 lines
The code is correct but it takes more seconds for me to understand. And static code analyzer do not understand it at all.
Quoted 1 line
Signed-off-by Oleksij Rempel <[email protected]>---
drivers/pinctrl/pinctrl-tegra30.c | 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)drivers/pinctrl/pinctrl-tegra30.c +0
-0@@ -658,8 +658,8 @@ static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl, > break; > } > }drivers/pinctrl/pinctrl-tegra30.c static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl, · line 662
Quoted 2 lines
drivers/pinctrl/pinctrl-tegra30.c +0
-2-/* if no matching drivegroup is found */ -if (i == ctrl->drvdata->num_drvgrps)+
Quoted 2 lines
Quoted 2 lines
drivers/pinctrl/pinctrl-tegra30.c +1
-0+if (!group)return 0;
Huh? This is a pretty standard idiom in C codebases to check if we broke out of a loop early.
Actually this change breaks the code, as this check is inside of an outer loop that doesn't reinitialize the group variable. So while the code as-is correctly checks if a group was found in the current iteration of the outer loop, after this patch it also matches a group that was found on a previous iteration of the outer loop.
This is a prime example where static checker warnings can prompt wrong fixes. Frankly codacy should smart up to correctly analyze the controlflow interdependency.
Sascha, please drop this patch.
Regards,
Lucas
_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v1 2/9] pinctrl: tegra30: fix "Possible null pointer dereference: group" warning
Quoted 26 lines · Lucas Stach
Am Dienstag, den 20.11.2018, 21:07 +0100 schrieb Oleksij Rempel:
Quoted 19 lines
The code is correct but it takes more seconds for me to understand. And static code analyzer do not understand it at all.
Quoted 1 line
Signed-off-by Oleksij Rempel <[email protected]>---
drivers/pinctrl/pinctrl-tegra30.c | 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)drivers/pinctrl/pinctrl-tegra30.c +0
-0@@ -658,8 +658,8 @@ static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl, > break; > } > }drivers/pinctrl/pinctrl-tegra30.c static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl, · line 662
Quoted 2 lines
drivers/pinctrl/pinctrl-tegra30.c +0
-2-/* if no matching drivegroup is found */ -if (i == ctrl->drvdata->num_drvgrps)+
Quoted 2 lines
Quoted 2 lines
drivers/pinctrl/pinctrl-tegra30.c +1
-0+if (!group)return 0;
Huh? This is a pretty standard idiom in C codebases to check if we broke out of a loop early.
Quoted 5 lines
Actually this change breaks the code, as this check is inside of an outer loop that doesn't reinitialize the group variable. So while the code as-is correctly checks if a group was found in the current iteration of the outer loop, after this patch it also matches a group that was found on a previous iteration of the outer loop.
Probably I still do not understand it:
static const struct pinctrl_tegra30_drvdata tegra124_drvdata = {
.pingrps = tegra124_pin_groups,
.num_pingrps = ARRAY_SIZE(tegra124_pin_groups),
.drvgrps = tegra124_drive_groups,
.num_drvgrps = ARRAY_SIZE(tegra124_drive_groups),
^^^^^ this is constant.
};static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl,
struct device_node *np)
{
const char *pins = NULL;
const struct tegra_drive_pingroup *group = NULL;
^^^^ here we init *group to NULL
int hsm = -1, schmitt = -1, pds = -1, pus = -1, srr = -1, srf = -1;
int i;
u32 __iomem *regaddr;
u32 val;
if (of_property_read_string(np, "nvidia,pins", &pins))
return 0;
for (i = 0; i < ctrl->drvdata->num_drvgrps; i++) {
^^^^ here we init i
if (!strcmp(pins, ctrl->drvdata->drvgrps[i].name)) {
group = &ctrl->drvdata->drvgrps[i];
^^^^^ -- only here group is not NULL
break;
}
}
/* if no matching drivegroup is found */
if (i == ctrl->drvdata->num_drvgrps)
^^^^^ if i == num_drvgrps, group is also NULL..
return 0;I don't see any technical problems. Or i do oversee some thing?
Quoted 3 lines
This is a prime example where static checker warnings can prompt wrong fixes. Frankly codacy should smart up to correctly analyze the controlflow interdependency.
Well, amount of real problems found by this code check is still higher. So why not to use it?
Signature · 4 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH v1 2/9] pinctrl: tegra30: fix "Possible null pointer dereference: group" warning
Am Mittwoch, den 21.11.2018, 10:40 +0100 schrieb Oleksij Rempel:
Quoted 67 lines · Lucas Stach
Quoted 25 lines
Am Dienstag, den 20.11.2018, 21:07 +0100 schrieb Oleksij Rempel:
Quoted 17 lines
The code is correct but it takes more seconds for me to understand. And static code analyzer do not understand it at all.
Quoted 1 line
Signed-off-by Oleksij Rempel <[email protected]>---
drivers/pinctrl/pinctrl-tegra30.c | 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)drivers/pinctrl/pinctrl-tegra30.c +0
-0@@ -658,8 +658,8 @@ static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl, > > > > > break; > > > > > } > > > > > }Quoted 6 lines
drivers/pinctrl/pinctrl-tegra30.c static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl, · line 662
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/pinctrl/pinctrl-tegra30.c +0
-1-/* if no matching drivegroup is found */ drivers/pinctrl/pinctrl-tegra30.c +0
-1-if (i == ctrl->drvdata->num_drvgrps)+
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/pinctrl/pinctrl-tegra30.c static int pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl, · line 662
Quoted 1 line
drivers/pinctrl/pinctrl-tegra30.c +1
-0+if (!group)return 0;
Quoted 8 lines
Huh? This is a pretty standard idiom in C codebases to check if we broke out of a loop early.
Actually this change breaks the code, as this check is inside of an outer loop that doesn't reinitialize the group variable. So while the code as-is correctly checks if a group was found in the current iteration of the outer loop, after this patch it also matches a group that was found on a previous iteration of the outer loop.
Probably I still do not understand it:
static const struct pinctrl_tegra30_drvdata tegra124_drvdata = { .pingrps = tegra124_pin_groups, .num_pingrps = ARRAY_SIZE(tegra124_pin_groups), .drvgrps = tegra124_drive_groups, .num_drvgrps = ARRAY_SIZE(tegra124_drive_groups), ^^^^^ this is constant. };static int
pinctrl_tegra30_set_drvstate(struct pinctrl_tegra30 *ctrl, struct device_node *np){ const char *pins = NULL; const struct tegra_drive_pingroup *group = NULL; ^^^^ here we init *group to NULL int hsm = -1, schmitt = -1, pds = -1, pus = -1, srr = -1, srf = -1; int i; u32 __iomem *regaddr; u32 val; if (of_property_read_string(np, "nvidia,pins", &pins))Quoted 1 line
return 0;for (i = 0; i < ctrl->drvdata->num_drvgrps; i++) { ^^^^ here we init iQuoted 5 lines
if (!strcmp(pins, ctrl->drvdata->drvgrps[i].name)) { group = &ctrl->drvdata->drvgrps[i]; ^^^^^ -- only here group is not NULL break; }
Quoted 7 lines
} /* if no matching drivegroup is found */ if (i == ctrl->drvdata->num_drvgrps) ^^^^^ if i == num_drvgrps, group is also NULL..Quoted 1 line
return 0;I don't see any technical problems. Or i do oversee some thing?
Sorry, I've looked at the wrong function. This patch doesn't break anything, but now makes the code inconsistent. In pinctrl_tegra30_set_state() this idiom is needed to make the code correct and I would rather have the groups searches consistent between the 2 functions.
Quoted 6 lines
Quoted 3 lines
This is a prime example where static checker warnings can prompt wrong fixes. Frankly codacy should smart up to correctly analyze the controlflow interdependency.
Well, amount of real problems found by this code check is still higher. So why not to use it?
I'm not arguing against static checkers, but I'm saying that it can sometimes be hard to get to the real issues, when the checker is producing a significant amount of false positives. And I'm against changing code just to make checkers happy, because every one of them gets something different wrong...
But whatever, the code is still correct, so the patch can stay.
Regards,
Lucas
_______________________________________________
barebox mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/barebox
PATCH 3/9 fs: closedir: remove uninitialized variable ret
In reply to Oleksij Rempel--- fs/fs.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
fs/fs.c +1 -3
@@ -2527,8 +2527,6 @@ EXPORT_SYMBOL(opendir);int closedir (DIR *dir) { -int ret; -if (!dir) { errno =EBADF ;return -EBADF ;
@@ -2536,7 +2534,7 @@ int closedir(DIR *dir)release_dir (dir); -return ret; +return 0 ; }EXPORT_SYMBOL (closedir);
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 4/9 fs: fix possible null pointer dereference of base.
In reply to Oleksij Rempelwe use base before checking if it is NULL
--- fs/fs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/fs.c +2 -1
@@ -1526,7 +1526,7 @@ static struct dentry *__lookup_hash(const struct qstr *name, {struct dentry *dentry;struct dentry *old; -struct inode *dir = base->d_inode; +struct inode *dir;if (!base)return ERR_PTR (-ENOENT );
@@ -1539,6 +1539,7 @@ static struct dentry *__lookup_hash(const struct qstr *name,if (unlikely (!dentry))return ERR_PTR (-ENOMEM ); + dir = base->d_inode; old = dir->i_op->lookup (dir, dentry, flags);if (IS_ERR (old)) {dput (dentry);
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 5/9 mfd: da9063: fix wrong NULL pointer test
In reply to Oleksij Rempelchecking for priv == NULL and using in this case priv->dev makes no sense.
--- drivers/mfd/da9063.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mfd/da9063.c +1 -1
@@ -201,7 +201,7 @@ static int da9062_device_init(struct da9063 *priv) priv->client1 =i2c_new_dummy (priv->client->adapter, priv->client->addr +1 ); -if (!priv) { +if (!priv->client1) {dev_warn (priv->dev,"failed to create bank 1 device\n" );/* TODO: return -EINVAL; i2c api does not return more * details */
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 6/9 usb: at91_udc: remove useless NULL check
In reply to Oleksij Rempelif _ep or ep is NULL, we would get at least in two places before this test a NULL pointer dereference.
--- drivers/usb/gadget/at91_udc.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/gadget/at91_udc.c +1 -2
@@ -288,8 +288,7 @@ static int at91_ep_enable(struct usb_ep *_ep,u16 maxpacket;u32 tmp; -if (!_ep || !ep - || !desc || ep->desc +if (!desc || ep->desc || _ep->name == ep0name || desc->bDescriptorType !=USB_DT_ENDPOINT || (maxpacket = le16_to_cpu(desc->wMaxPacketSize)) == 0
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 7/9 mtd: atmel_nand: remove erroneous case
In reply to Oleksij RempelIf mm would be 15, then we will corrupt the stack, since p[15]. And as input for this function, mm has only two variants 13 and 14.
--- drivers/mtd/nand/atmel_nand.c | 1 - 1 file changed, 1 deletion(-)
drivers/mtd/nand/atmel_nand.c +0 -1
@@ -788,7 +788,6 @@ static int pmecc_build_galois_table(unsigned int mm, int16_t *index_of,case 3 :case 4 :case 6 : -case 15 : p[1 ] =1 ;break ;case 5 :
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 8/9 usb: musb: fix possible out of bounds access
In reply to Oleksij RempelEither the condition 'epnum>=((u8)16)' is redundant or the array 'musb->endpoints[16]' is accessed at index 16, which is out of bounds.
--- drivers/usb/musb/musb_gadget_ep0.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/usb/musb/musb_gadget_ep0.c +6 -1
@@ -110,6 +110,11 @@ static int service_tx_status_request(break ; } +if (epnum >=MUSB_C_NUM_EPS ) { + handled = -EINVAL ; +break ; + } + is_in = epnum &USB_DIR_IN ;if (is_in) { epnum &=0x0f ;
@@ -119,7 +124,7 @@ static int service_tx_status_request( } regs = musb->endpoints[epnum].regs; -if (epnum >=MUSB_C_NUM_EPS || !ep->desc) { +if (!ep->desc) { handled = -EINVAL ;break ; }
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
PATCH 9/9 lib: lodepng: remove useless test
In reply to Oleksij RempelEither the condition 'code_ll==(unsigned int)(-1)' is redundant or the array 'LENGTHBASE[29]' is accessed at index 4294967038, which is out of bounds.
On other hand this condition is tested within other scope:
"if(code_ll >= FIRST_LENGTH_CODE_INDEX && code_ll <= LAST_LENGTH_CODE_INDEX)" with is already limited to (code_ll >= 257 && code_ll <= 285),
so it can't be -1 any way.
--- lib/gui/lodepng.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
lib/gui/lodepng.c +1 -7
@@ -1140,13 +1140,7 @@ static unsigned inflateHuffmanBlock(ucvector* out, const unsigned char* in, size code_d =huffmanDecodeSymbol (in, bp, &tree_d, inbitlength);if (code_d >29 ) { -if (code_ll == (unsigned )(-1 ))/*huffmanDecodeSymbol returns (unsigned)(-1) in case of error*/ - { -/*return error code 10 or 11 depending on the situation that happened in huffmanDecodeSymbol -(10=no endcode, 11=wrong jump outside of tree)*/ - error = (*bp) > inlength *8 ?10 :11 ; - } -else error =18 ;/*error: invalid distance code (30-31 are never used)*/ + error =18 ;/*error: invalid distance code (30-31 are never used)*/ break ; } distance =DISTANCEBASE [code_d];
Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
Sascha Hauer
AppliedQuoted 14 lines · Oleksij Rempel
next round of random fixes reported by codacy:
https://app.codacy.com/app/olerem/barebox/issues/index?bid=9803255Oleksij Rempel (9):
net: e1000: fix "Uninitialized variable:phy_data" warning
pinctrl: tegra30: fix "Possible null pointer dereference: group"
warning
fs: closedir: remove uninitialized variable ret
fs: fix possible null pointer dereference of base.
mfd: da9063: fix wrong NULL pointer test
usb:at91_udc: remove useless NULL check
mtd:atmel_nand: remove erroneous case
usb: musb: fix possible out of bounds access
lib: lodepng: remove useless test
Applied, thanks
Sascha
Quoted 20 lines
drivers/mfd/da9063.c | 2 +- drivers/mtd/nand/atmel_nand.c | 1 - drivers/net/e1000/main.c | 2 +- drivers/pinctrl/pinctrl-tegra30.c | 4 ++-- drivers/usb/gadget/at91_udc.c | 3 +-- drivers/usb/musb/musb_gadget_ep0.c | 7 ++++++- fs/fs.c | 7 +++---- lib/gui/lodepng.c | 8 +------- 8 files changed, 15 insertions(+), 19 deletions(-)Signature · 7 lines
2.17.1 _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox
Signature · 9 lines
Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list [email protected] http://lists.infradead.org/mailman/listinfo/barebox