* [U-Boot] [PATCH v2] Prevented possible null dereference.
@ 2019-08-26 12:54 Niv Shetrit
2019-12-05 15:15 ` Tom Rini
0 siblings, 1 reply; 3+ messages in thread
From: Niv Shetrit @ 2019-08-26 12:54 UTC (permalink / raw)
To: u-boot
Signed-off-by: Niv Shetrit <niv.shetrit@altair-semi.com>
---
common/cli_hush.c | 73 ++++++++++++++++++++++++-----------------------
1 file changed, 38 insertions(+), 35 deletions(-)
diff --git a/common/cli_hush.c b/common/cli_hush.c
index 8f86e4aa4a..c14302c3ad 100644
--- a/common/cli_hush.c
+++ b/common/cli_hush.c
@@ -3539,41 +3539,44 @@ static char *insert_var_value_sub(char *inp, int tag_subst)
}
inp = ++p;
/* find the ending marker */
- p = strchr(inp, SPECIAL_VAR_SYMBOL);
- *p = '\0';
- /* look up the value to substitute */
- if ((p1 = lookup_param(inp))) {
- if (tag_subst)
- len = res_str_len + strlen(p1) + 2;
- else
- len = res_str_len + strlen(p1);
- res_str = xrealloc(res_str, (1 + len));
- if (tag_subst) {
- /*
- * copy the variable value to the result
- * string
- */
- strcpy((res_str + res_str_len + 1), p1);
-
- /*
- * mark the replaced text to be accepted as
- * is
- */
- res_str[res_str_len] = SUBSTED_VAR_SYMBOL;
- res_str[res_str_len + 1 + strlen(p1)] =
- SUBSTED_VAR_SYMBOL;
- } else
- /*
- * copy the variable value to the result
- * string
- */
- strcpy((res_str + res_str_len), p1);
-
- res_str_len = len;
- }
- *p = SPECIAL_VAR_SYMBOL;
- inp = ++p;
- done = 1;
+ p = strchr(inp, SPECIAL_VAR_SYMBOL)
+ if (p != NULL) {
+ *p = '\0';
+ /* look up the value to substitute */
+ p1 = lookup_param(inp)
+ if (p1 != NULL) {
+ if (tag_subst)
+ len = res_str_len + strlen(p1) + 2;
+ else
+ len = res_str_len + strlen(p1);
+ res_str = xrealloc(res_str, (1 + len));
+ if (tag_subst) {
+ /*
+ * copy the variable value to the
+ * result string
+ */
+ strcpy((res_str + res_str_len + 1), p1);
+
+ /*
+ * mark the replaced text to be
+ * accepted as is
+ */
+ res_str[res_str_len] = SUBSTED_VAR_SYMBOL;
+ res_str[res_str_len + 1 + strlen(p1)] =
+ SUBSTED_VAR_SYMBOL;
+ } else
+ /*
+ * copy the variable value to the result
+ * string
+ */
+ strcpy((res_str + res_str_len), p1);
+
+ res_str_len = len;
+ }
+ *p = SPECIAL_VAR_SYMBOL;
+ inp = ++p;
+ done = 1;
+ }
}
if (done) {
res_str = xrealloc(res_str, (1 + res_str_len + strlen(inp)));
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [U-Boot] [PATCH v2] Prevented possible null dereference.
2019-08-26 12:54 [U-Boot] [PATCH v2] Prevented possible null dereference Niv Shetrit
@ 2019-12-05 15:15 ` Tom Rini
0 siblings, 0 replies; 3+ messages in thread
From: Tom Rini @ 2019-12-05 15:15 UTC (permalink / raw)
To: u-boot
On Mon, Aug 26, 2019 at 05:54:59AM -0700, Niv Shetrit wrote:
> Signed-off-by: Niv Shetrit <niv.shetrit@altair-semi.com>
Sorry for taking so long to get back to this. A few problems. And
re-ordering the diff to make explanation clearer:
> ---
> common/cli_hush.c | 73 ++++++++++++++++++++++++-----------------------
> 1 file changed, 38 insertions(+), 35 deletions(-)
>
> diff --git a/common/cli_hush.c b/common/cli_hush.c
> index 8f86e4aa4a..c14302c3ad 100644
> --- a/common/cli_hush.c
> +++ b/common/cli_hush.c
> @@ -3539,41 +3539,44 @@ static char *insert_var_value_sub(char *inp, int tag_subst)
> }
> inp = ++p;
> /* find the ending marker */
> - p = strchr(inp, SPECIAL_VAR_SYMBOL);
> + p = strchr(inp, SPECIAL_VAR_SYMBOL)
You forgot the trailing ';' so this can't compile and wasn't tested.
> - *p = '\0';
> - /* look up the value to substitute */
> - if ((p1 = lookup_param(inp))) {
> + p1 = lookup_param(inp)
> + if (p1 != NULL) {
Again you forgot a trailing ';' and you've expanded equivalent
statements. If we had a single set of parenthesis, in other words:
if (p1 = lookup_param(inp)) {
...
it would still check if we got NULL/non-NULL, but gcc would complain:
warning: suggest parentheses around assignment used as truth value [-Wparenthese]
and with the parenthesis we have, it does what we want here.
But we have the extra set of parenthesis to say we care about the result
of that assignment and are doing this on purpose.
Which brings us to the other functional change (which is more visible
with git show -w or similar, to ignore whitespace changes):
> + p = strchr(inp, SPECIAL_VAR_SYMBOL)
> + if (p != NULL) {
> + *p = '\0';
> + /* look up the value to substitute */
> + p1 = lookup_param(inp)
[I've re-included the code above back here for more context]
You're making sure that if strchr gives us NULL back, we don't call
lookup_param. But lookup_param handles being passed NULL correctly.
So in sum, there's no problem here. Thanks for looking around.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20191205/51752ed2/attachment.sig>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [U-Boot] [PATCH v2] Prevented possible null dereference.
@ 2019-08-26 16:55 Niv Shetrit
0 siblings, 0 replies; 3+ messages in thread
From: Niv Shetrit @ 2019-08-26 16:55 UTC (permalink / raw)
To: u-boot
Signed-off-by: Niv Shetrit <niv.shetrit@altair-semi.com>
---
common/cli_hush.c | 73 ++++++++++++++++++++++++-----------------------
1 file changed, 38 insertions(+), 35 deletions(-)
diff --git a/common/cli_hush.c b/common/cli_hush.c
index 8f86e4aa4a..c14302c3ad 100644
--- a/common/cli_hush.c
+++ b/common/cli_hush.c
@@ -3539,41 +3539,44 @@ static char *insert_var_value_sub(char *inp, int tag_subst)
}
inp = ++p;
/* find the ending marker */
- p = strchr(inp, SPECIAL_VAR_SYMBOL);
- *p = '\0';
- /* look up the value to substitute */
- if ((p1 = lookup_param(inp))) {
- if (tag_subst)
- len = res_str_len + strlen(p1) + 2;
- else
- len = res_str_len + strlen(p1);
- res_str = xrealloc(res_str, (1 + len));
- if (tag_subst) {
- /*
- * copy the variable value to the result
- * string
- */
- strcpy((res_str + res_str_len + 1), p1);
-
- /*
- * mark the replaced text to be accepted as
- * is
- */
- res_str[res_str_len] = SUBSTED_VAR_SYMBOL;
- res_str[res_str_len + 1 + strlen(p1)] =
- SUBSTED_VAR_SYMBOL;
- } else
- /*
- * copy the variable value to the result
- * string
- */
- strcpy((res_str + res_str_len), p1);
-
- res_str_len = len;
- }
- *p = SPECIAL_VAR_SYMBOL;
- inp = ++p;
- done = 1;
+ p = strchr(inp, SPECIAL_VAR_SYMBOL)
+ if (p != NULL) {
+ *p = '\0';
+ /* look up the value to substitute */
+ p1 = lookup_param(inp)
+ if (p1 != NULL) {
+ if (tag_subst)
+ len = res_str_len + strlen(p1) + 2;
+ else
+ len = res_str_len + strlen(p1);
+ res_str = xrealloc(res_str, (1 + len));
+ if (tag_subst) {
+ /*
+ * copy the variable value to the
+ * result string
+ */
+ strcpy((res_str + res_str_len + 1), p1);
+
+ /*
+ * mark the replaced text to be
+ * accepted as is
+ */
+ res_str[res_str_len] = SUBSTED_VAR_SYMBOL;
+ res_str[res_str_len + 1 + strlen(p1)] =
+ SUBSTED_VAR_SYMBOL;
+ } else
+ /*
+ * copy the variable value to the result
+ * string
+ */
+ strcpy((res_str + res_str_len), p1);
+
+ res_str_len = len;
+ }
+ *p = SPECIAL_VAR_SYMBOL;
+ inp = ++p;
+ done = 1;
+ }
}
if (done) {
res_str = xrealloc(res_str, (1 + res_str_len + strlen(inp)));
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-12-05 15:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-26 12:54 [U-Boot] [PATCH v2] Prevented possible null dereference Niv Shetrit
2019-12-05 15:15 ` Tom Rini
2019-08-26 16:55 Niv Shetrit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.