Re: [PATCH] IOMUX: Fix access past end of console_devices
On Wed, Mar 30, 2022 at 01:25:59PM -0400, Sean Anderson wrote: > On 3/30/22 1:13 PM, Andy Shevchenko wrote: > > On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson wrote: > > > > Also I don't like to have workarounds for the broken tools. > > But if you still want to have something, what about rather this > > > > > #define for_each_console_dev(i, file, dev) \ > > > - for (i = 0, dev = console_devices[file][i]; \ > > > -i < cd_count[file];\ > > > -i++, dev = console_devices[file][i]) > > > + for (i = 0; i < cd_count[file] && \ > > > + (dev = console_devices[file][i]); i++) > > > > for (i = 0, dev = console_devices[file][0]; \ > > i < cd_count[file];\ > > i++, dev = console_devices[file][i]) > > > > ? > > > > Or if it's still complains > > > > for (i = 0, dev = cd_count[file] ? console_devices[file][0] : NULL; > > \ > > i < cd_count[file];\ > > i++, dev = console_devices[file][i]) > > > > ? > > The problem is not the first assignment but the last. Consider the case when > cd_count[file] = 1 Following that logic the first one is also problematic when cd_count[file] == 0. > i = 0, dev = console_devices[file][0]; // OK > i < cd_count[file] // 0 < 1 > // loop body > i++, dev = console_devices[file][1] // Oops, past the end > i < cd_count[file] // 1 < 1, loop exit I don't see good solution. :-( Maybe moving to dev as a loop variable and having NULL terminated arrays should fix that. For now probably your solution is a good compromise. But, please rewrite it to have all three more visible in a for-loop: for (i = 0; \ i < cd_count[file] && (dev = console_devices[file][i]);\ i++) -- With Best Regards, Andy Shevchenko
Re: [PATCH] IOMUX: Fix access past end of console_devices
On 3/30/22 1:13 PM, Andy Shevchenko wrote: On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson wrote: Also I don't like to have workarounds for the broken tools. But if you still want to have something, what about rather this #define for_each_console_dev(i, file, dev) \ - for (i = 0, dev = console_devices[file][i]; \ -i < cd_count[file];\ -i++, dev = console_devices[file][i]) + for (i = 0; i < cd_count[file] && \ + (dev = console_devices[file][i]); i++) for (i = 0, dev = console_devices[file][0]; \ i < cd_count[file];\ i++, dev = console_devices[file][i]) ? Or if it's still complains for (i = 0, dev = cd_count[file] ? console_devices[file][0] : NULL; \ i < cd_count[file];\ i++, dev = console_devices[file][i]) ? The problem is not the first assignment but the last. Consider the case when cd_count[file] = 1 i = 0, dev = console_devices[file][0]; // OK i < cd_count[file] // 0 < 1 // loop body i++, dev = console_devices[file][1] // Oops, past the end i < cd_count[file] // 1 < 1, loop exit --Sean
Re: [PATCH] IOMUX: Fix access past end of console_devices
On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson wrote: Also I don't like to have workarounds for the broken tools. But if you still want to have something, what about rather this > #define for_each_console_dev(i, file, dev) \ > - for (i = 0, dev = console_devices[file][i]; \ > -i < cd_count[file];\ > -i++, dev = console_devices[file][i]) > + for (i = 0; i < cd_count[file] && \ > + (dev = console_devices[file][i]); i++) for (i = 0, dev = console_devices[file][0]; \ i < cd_count[file];\ i++, dev = console_devices[file][i]) ? Or if it's still complains for (i = 0, dev = cd_count[file] ? console_devices[file][0] : NULL; \ i < cd_count[file];\ i++, dev = console_devices[file][i]) ? -- With Best Regards, Andy Shevchenko
Re: [PATCH] IOMUX: Fix access past end of console_devices
On 3/30/22 1:07 PM, Andy Shevchenko wrote:
On Wed, Mar 30, 2022 at 8:01 PM Andy Shevchenko
wrote:
On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson wrote:
...
#define for_each_console_dev(i, file, dev) \
- for (i = 0, dev = console_devices[file][i]; \
When we enter the loop, the dev is assigned and perhaps valid
-i < cd_count[file];\
-i++, dev = console_devices[file][i])
+ for (i = 0; i < cd_count[file] && \
Not the case anymore.
+ (dev = console_devices[file][i]); i++)
On the second look, it seems a bit unusual, but for loop checks the
condition before entering and in such case the dev will be assigned if
the count is greater than 0.
So, basically the difference is that dev is left completely
uninitialized in case of count==0. However, it may not be a problem.
Well, in such a case, the value of console_devices[file][i] is bogus
anyway, so stack garbage is just as good.
If it turns out to be a problem, this can always be rewritten to
for (i = 0 dev = NULL; i < cd_count[file] && \
(dev = console_devices[file][i]); i++)
Anyways, I would rather see better written for-loop that we see the iterations
Sorry, I'm not sure what you mean by this...
Ideally this loop would be written like
for (i = 0 dev = NULL; i < cd_count[file]; i++) {
dev = console_devices[file][i]);
/* loop body */
}
which is much more obviously correct. But since this macro must use the
following
block as the loop body, it's trickier to do.
--Sean
Re: [PATCH] IOMUX: Fix access past end of console_devices
On Wed, Mar 30, 2022 at 8:05 PM Sean Anderson wrote: > On 3/30/22 1:01 PM, Andy Shevchenko wrote: > > On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson wrote: ... > >> #define for_each_console_dev(i, file, dev) \ > >> - for (i = 0, dev = console_devices[file][i]; \ > > > > When we enter the loop, the dev is assigned and perhaps valid > > > >> -i < cd_count[file];\ > >> -i++, dev = console_devices[file][i]) > >> + for (i = 0; i < cd_count[file] && \ > > > > Not the case anymore. > > The loop condition is evaluated before we enter the loop, > which includes the first assignment to dev. Yeah, I just sent a reply to my reply :-) > >> + (dev = console_devices[file][i]); i++) So, what I don't like is exactly this hidenness, which I have stumbled upon. -- With Best Regards, Andy Shevchenko
Re: [PATCH] IOMUX: Fix access past end of console_devices
On Wed, Mar 30, 2022 at 8:01 PM Andy Shevchenko wrote: > On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson wrote: ... > > #define for_each_console_dev(i, file, dev) \ > > - for (i = 0, dev = console_devices[file][i]; \ > > When we enter the loop, the dev is assigned and perhaps valid > > > -i < cd_count[file];\ > > -i++, dev = console_devices[file][i]) > > + for (i = 0; i < cd_count[file] && \ > > Not the case anymore. > > > + (dev = console_devices[file][i]); i++) On the second look, it seems a bit unusual, but for loop checks the condition before entering and in such case the dev will be assigned if the count is greater than 0. So, basically the difference is that dev is left completely uninitialized in case of count==0. However, it may not be a problem. Anyways, I would rather see better written for-loop that we see the iterations -- With Best Regards, Andy Shevchenko
Re: [PATCH] IOMUX: Fix access past end of console_devices
On 3/30/22 1:01 PM, Andy Shevchenko wrote:
On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson wrote:
We should only access console_devices[file][i] once we have checked that
i < cd_count[file]. Otherwise, we will access uninitialized memory at the end of
the loop. console_devices[file][i] should not be NULL, but putting the
assignment in the loop condition allows us to ensure that i is checked
beforehand. This isn't a bug, but it does make valgrind stop complaining.
Fixes: 400797cad3 ("IOMUX: Split out for_each_console_dev() helper macro")
Has this been tested? See below.
Yes.
...
#define for_each_console_dev(i, file, dev) \
- for (i = 0, dev = console_devices[file][i]; \
When we enter the loop, the dev is assigned and perhaps valid
-i < cd_count[file];\
-i++, dev = console_devices[file][i])
+ for (i = 0; i < cd_count[file] && \
Not the case anymore.
The loop condition is evaluated before we enter the loop,
which includes the first assignment to dev.
--Sean
+ (dev = console_devices[file][i]); i++)
Re: [PATCH] IOMUX: Fix access past end of console_devices
On Wed, Mar 30, 2022 at 7:49 PM Sean Anderson wrote:
>
> We should only access console_devices[file][i] once we have checked that
> i < cd_count[file]. Otherwise, we will access uninitialized memory at the end
> of
> the loop. console_devices[file][i] should not be NULL, but putting the
> assignment in the loop condition allows us to ensure that i is checked
> beforehand. This isn't a bug, but it does make valgrind stop complaining.
> Fixes: 400797cad3 ("IOMUX: Split out for_each_console_dev() helper macro")
Has this been tested? See below.
...
> #define for_each_console_dev(i, file, dev) \
> - for (i = 0, dev = console_devices[file][i]; \
When we enter the loop, the dev is assigned and perhaps valid
> -i < cd_count[file];\
> -i++, dev = console_devices[file][i])
> + for (i = 0; i < cd_count[file] && \
Not the case anymore.
> + (dev = console_devices[file][i]); i++)
--
With Best Regards,
Andy Shevchenko
[PATCH] IOMUX: Fix access past end of console_devices
We should only access console_devices[file][i] once we have checked that
i < cd_count[file]. Otherwise, we will access uninitialized memory at the end of
the loop. console_devices[file][i] should not be NULL, but putting the
assignment in the loop condition allows us to ensure that i is checked
beforehand. This isn't a bug, but it does make valgrind stop complaining.
Fixes: 400797cad3 ("IOMUX: Split out for_each_console_dev() helper macro")
Signed-off-by: Sean Anderson
---
include/iomux.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/iomux.h b/include/iomux.h
index 37f5f6dee6..1046df133b 100644
--- a/include/iomux.h
+++ b/include/iomux.h
@@ -25,9 +25,8 @@ extern struct stdio_dev **console_devices[MAX_FILES];
extern int cd_count[MAX_FILES];
#define for_each_console_dev(i, file, dev) \
- for (i = 0, dev = console_devices[file][i]; \
-i < cd_count[file];\
-i++, dev = console_devices[file][i])
+ for (i = 0; i < cd_count[file] && \
+ (dev = console_devices[file][i]); i++)
int iomux_match_device(struct stdio_dev **, const int, struct stdio_dev *);
int iomux_doenv(const int, const char *);
--
2.35.1
