env: fix crash using default -f -a
PATCH 1/1 env: fix crash using default -f -a
newval pointer is not checked before its usage,
inside env_check_apply().
--- common/cmd_nvedit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
common/cmd_nvedit.c +3 -3
@@ -272,7 +272,7 @@ int env_check_apply(const char *name, const char *oldval,/* * Switch to new baudrate if new baudrate is supported */ -if (strcmp (name,"baudrate" ) ==0 ) { +if ((strcmp (name,"baudrate" ) ==0 ) && (newval !=NULL )) {int baudrate =simple_strtoul (newval,NULL ,10 );int i;for (i =0 ; i <N_BAUDRATES ; ++i) {
@@ -308,12 +308,12 @@ int env_check_apply(const char *name, const char *oldval,* Some variables should be updated when the corresponding * entry in the environment is changed */ -if (strcmp (name,"loadaddr" ) ==0 ) { +if ((strcmp (name,"loadaddr" ) ==0 ) && (newval !=NULL )) { load_addr =simple_strtoul (newval,NULL ,16 );return 0 ; } #if defined(CONFIG_CMD_NET) - else if (strcmp(name, "bootfile") == 0) { + else if ((strcmp(name, "bootfile") == 0) && (newval != NULL)) {copy_filename (BootFile, newval,sizeof (BootFile));return 0 ; }
Signature · 1 line
1.7.9.5
PATCH 1/1 env: fix crash using default -f -a
Quoted 5 lines · Stefano Babic
newval pointer is not checked before its usage,
insideenv_check_apply().Signed-off-by Stefano Babic <[email protected]>CC gerlando.falauto at keymile.com
Some egg on my face for not catching this to start with, confirmed problem and fix here.
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121004/c35cedbf/attachment.pgp>
PATCH 1/1 env: fix crash using default -f -a
In reply to Stefano BabicHi Stefano,
thanks for reporting and providing a fix for this.
I'm very sorry for introducing this problem and for the late response.
Please see my comments below.
[As a side node, I couldn't really reproduce the issue neither on PowerPC nor on ARM (though simple_strtoul should legitimately crash) as apparently accessing NULL locations doesn't really bother our architectures...]
Quoted 14 lines · Stefano Babic
newval pointer is not checked before its usage,
insideenv_check_apply().Signed-off-by Stefano Babic<[email protected]>CC gerlando.falauto at keymile.com--- common/cmd_nvedit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)common/cmd_nvedit.c +0
-0
common/cmd_nvedit.c int env_check_apply(const char *name, const char *oldval, · line 275
Quoted 5 lines
common/cmd_nvedit.c +1
-1@@ -272,7 +272,7 @@ int env_check_apply(const char *name, const char *oldval,/* * Switch to new baudrate if new baudrate is supported */ -if (strcmp (name,"baudrate" ) ==0 ) { +if ((strcmp (name,"baudrate" ) ==0 )&& (newval !=NULL )) {
You're right in that the problem here is due to the function explicitly being called with a NULL value.
My idea was to have "env default" perform some sort of cleanup before assigning the actual values from the default environment (if any). So by adding these extra checks you're subtly canceling those efforts. So I would rather provide a default value to prevent the crashes, instead of skipping settings altogether. Posting a patch in a few minutes.
Whether it's legitimate to perform these "cleanups", well that's of course open for discussion.
But I believe it doesn't make much sense to call a function and pass an argument which is sort of guaranteed to have no effect whatsoever.
Quoted 8 lines
common/cmd_nvedit.c +0
-1int baudrate =simple_strtoul (newval,NULL ,10 );int i;for (i =0 ; i<N_BAUDRATES ; ++i) {@@ -308,12 +308,12 @@ int env_check_apply(const char *name, const char *oldval,* Some variables should be updated when the corresponding * entry in the environment is changed */ -if (strcmp (name,"loadaddr" ) ==0 ) {
common/cmd_nvedit.c int env_check_apply(const char *name, const char *oldval, · line 319
Quoted 10 lines
common/cmd_nvedit.c +2
-1+if ((strcmp (name,"loadaddr" ) ==0 )&& (newval !=NULL )) { load_addr =simple_strtoul (newval,NULL ,16 );return 0 ; } #if defined(CONFIG_CMD_NET) - else if (strcmp(name, "bootfile") == 0) { + else if ((strcmp(name, "bootfile") == 0)&& (newval != NULL)) {copy_filename (BootFile, newval,sizeof (BootFile));return 0 ; }
Thank you and sorry again,
Gerlando
PATCH 1/1 env: fix crash using default -f -a
Quoted 2 lines · Gerlando Falauto
Hi Stefano,
Hi Gerlando,
Quoted 9 lines
thanks for reporting and providing a fix for this.
I'm very sorry for introducing this problem and for the late response.Please see my comments below.
[As a side node, I couldn't really reproduce the issue neither on PowerPC nor on ARM (though
simple_strtoulshould legitimately crash) as apparently accessing NULL locations doesn't really bother our architectures...]
Right. For PowerPC, Address 0 is a valid address in memory and the system does not crash. However, I tested on a TI's ARM and address zero is not valid.
Quoted 11 lines · Stefano Babic
Quoted 9 lines
newval pointer is not checked before its usage,
insideenv_check_apply().Signed-off-by Stefano Babic<[email protected]>CC gerlando.falauto at keymile.com--- common/cmd_nvedit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Quoted 16 lines
common/cmd_nvedit.c int env_check_apply(const char *name, const char · line 276
Quoted 11 lines
common/cmd_nvedit.c +1
-1@@ -272,7 +272,7 @@ int env_check_apply(const char *name, const char *oldval,/* * Switch to new baudrate if new baudrate is supported */ - if (strcmp(name, "baudrate") == 0) { + if ((strcmp(name, "baudrate") == 0)&& (newval != NULL)) {You're right in that the problem here is due to the function explicitly being called with a NULL value.
My idea was to have "env default" perform some sort of cleanup before assigning the actual values from the default environment (if any).
I have seen, I have not fully understood. My idea is that, if someone want to restore the environment to the default values, no actions should be taken, else the "default" values cannot be really restored.
Quoted 1 line
So by adding these extra checks you're subtly canceling those efforts.
That is correct, but in any case a check for a NULL pointer should be done before accessing to the pointer, independently what we want to do with it.
Quoted 2 lines
So I would rather provide a default value to prevent the crashes, instead of skipping settings altogether. Posting a patch in a few minutes.
Ok, I will review it. In any case, we need a fix for the coming release.
Quoted 3 lines
Whether it's legitimate to perform these "cleanups", well that's of course open for discussion.
Indeed.
Quoted 2 lines
But I believe it doesn't make much sense to call a function and pass an argument which is sort of guaranteed to have no effect whatsoever.
Right. It is the same as calling the function with do_apply=0.
Quoted 12 lines
Quoted 12 lines
common/cmd_nvedit.c +0
-0int baudrate =simple_strtoul (newval,NULL ,10 );int i;for (i = 0; i< N_BAUDRATES; ++i) {common/cmd_nvedit.c +1
-1@@ -308,12 +308,12 @@ int env_check_apply(const char *name, const char*oldval, * Some variables should be updated when the corresponding * entry in the environment is changed */ -if (strcmp (name,"loadaddr" ) ==0 ) { +if ((strcmp (name,"loadaddr" ) ==0 )&& (newval !=NULL )) { load_addr =simple_strtoul (newval,NULL ,16 );
Quoted 9 lines
Quoted 8 lines
common/cmd_nvedit.c +1
-1return 0 ; } #if defined(CONFIG_CMD_NET) - else if (strcmp(name, "bootfile") == 0) { + else if ((strcmp(name, "bootfile") == 0)&& (newval != NULL)) {copy_filename (BootFile, newval,sizeof (BootFile));return 0 ;}
Best regards,
Stefano
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================
PATCH 1/1 env: fix crash using default -f -a
env default -a -f calls env_check_apply on all existing environment variables with a NULL value for "newval" as a way of cleaning up. This causes string manipulation functions to crash on most architectures. So replace a NULL argument with an empty string.
--- common/cmd_nvedit.c | 3 +++ 1 file changed, 3 insertions(+)
common/cmd_nvedit.c +3 -0
@@ -213,6 +213,9 @@ int env_check_apply(const char *name, const char *oldval, {int console =-1 ; +/* Default value for NULL to protect string-manipulating functions */ + newval = newval ? :"" ; +/* Check for console redirection */ if (strcmp (name,"stdin" ) ==0 ) console = stdin;
Signature · 1 line
1.7.10.1
PATCH 1/1 env: fix crash using default -f -a
Quoted 17 lines · Gerlando Falauto
env default -a -f calls
env_check_applyon all existing environment variables with a NULL value for "newval" as a way of cleaning up. This causes string manipulation functions to crash on most architectures. So replace a NULL argument with an empty string.Reported-By Stefano Babic <[email protected]>Signed-off-by Gerlando Falauto <[email protected]>--- common/cmd_nvedit.c | 3 +++ 1 file changed, 3 insertions(+)common/cmd_nvedit.c +0
-0
common/cmd_nvedit.c int env_check_apply(const char *name, const char *oldval, · line 221
Quoted 9 lines
common/cmd_nvedit.c +3
-0@@ -213,6 +213,9 @@ int env_check_apply(const char *name, const char *oldval, {int console =-1 ; +/* Default value for NULL to protect string-manipulating functions */ + newval = newval ? :"" ; +/* Check for console redirection */ if (strcmp (name,"stdin" ) ==0 ) console = stdin;
It fixes the issue.
Best regards,
Stefano
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================
PATCH 1/1 env: fix crash using default -f -a
In reply to Gerlando FalautoQuoted 7 lines · Gerlando Falauto
env default -a -f calls
env_check_applyon all existing environment variables with a NULL value for "newval" as a way of cleaning up. This causes string manipulation functions to crash on most architectures. So replace a NULL argument with an empty string.Reported-By Stefano Babic <[email protected]>Signed-off-by Gerlando Falauto <[email protected]>
Applied to u-boot/master, thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121005/9919bbd3/attachment.pgp>
PATCH 1/1 env: fix crash using default -f -a
In reply to Stefano BabicHi Stefano,
Quoted 11 lines · Stefano Babic
Quoted 2 lines · Gerlando Falauto
Hi Stefano,
Hi Gerlando,
Quoted 5 lines
thanks for reporting and providing a fix for this.
I'm very sorry for introducing this problem and for the late response.Please see my comments below.
Quoted 7 lines
Quoted 4 lines
[As a side node, I couldn't really reproduce the issue neither on PowerPC nor on ARM (though
simple_strtoulshould legitimately crash) as apparently accessing NULL locations doesn't really bother our architectures...]Right. For PowerPC, Address 0 is a valid address in memory and the system does not crash.
I didn't know that, thanks.
Quoted 31 lines
However, I tested on a TI's ARM and address zero
is not valid.Quoted 11 lines · Stefano Babic
Quoted 9 lines
newval pointer is not checked before its usage,
insideenv_check_apply().Signed-off-by Stefano Babic<[email protected]>CC gerlando.falauto at keymile.com--- common/cmd_nvedit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)Quoted 16 lines
common/cmd_nvedit.c int env_check_apply(const char *name, const char · line 276
Quoted 11 lines
common/cmd_nvedit.c +1
-1@@ -272,7 +272,7 @@ int env_check_apply(const char *name, const char *oldval,/* * Switch to new baudrate if new baudrate is supported */ - if (strcmp(name, "baudrate") == 0) { + if ((strcmp(name, "baudrate") == 0)&& (newval != NULL)) {You're right in that the problem here is due to the function explicitly being called with a NULL value.
My idea was to have "env default" perform some sort of cleanup before assigning the actual values from the default environment (if any).
Quoted 3 lines
I have seen, I have not fully understood. My idea is that, if someone want to restore the environment to the default values, no actions should be taken, else the "default" values cannot be really restored.
My idea here was to "apply" the changes as soon as the environment gets changed, like it would happen with a manual "setenv"/"env set". So for instance, if you switch from a custom baudrate back to the default baudrate, you get a message prompting you to switch to the new baudrate (which actually *DOES* change on the console).
Whether that should also be performed as a "cleanup" action upon restoring defaults, well I am not sure...
Quoted 5 lines
Quoted 1 line
So by adding these extra checks you're subtly canceling those efforts.
That is correct, but in any case a check for a NULL pointer should be done before accessing to the pointer, independently what we want to do with it.
Absolutely. See my proposed patch.
Quoted 4 lines
Quoted 2 lines
So I would rather provide a default value to prevent the crashes, instead of skipping settings altogether. Posting a patch in a few minutes.
Ok, I will review it. In any case, we need a fix for the coming release.
Absolutely. Once again, I cannot apologize and thank you enough for reporting and digging into this.
I assumed providing an own patch would make more sense than asking you to review yours, not sure whether it is popular practice to do so.
Quoted 9 lines
Quoted 2 lines
Whether it's legitimate to perform these "cleanups", well that's of course open for discussion.
Indeed.
Quoted 2 lines
But I believe it doesn't make much sense to call a function and pass an argument which is sort of guaranteed to have no effect whatsoever.
Right. It is the same as calling the function with do_apply=0.
Right, but since the return value is also ignored, what's the point in calling it in the first place?
Quoted 22 lines
Quoted 12 lines
Quoted 12 lines
common/cmd_nvedit.c +0
-0int baudrate =simple_strtoul (newval,NULL ,10 );int i;for (i = 0; i< N_BAUDRATES; ++i) {common/cmd_nvedit.c +1
-1@@ -308,12 +308,12 @@ int env_check_apply(const char *name, const char*oldval, * Some variables should be updated when the corresponding * entry in the environment is changed */ -if (strcmp (name,"loadaddr" ) ==0 ) { +if ((strcmp (name,"loadaddr" ) ==0 )&& (newval !=NULL )) { load_addr =simple_strtoul (newval,NULL ,16 );Quoted 9 lines
Quoted 8 lines
common/cmd_nvedit.c +1
-1return 0 ; } #if defined(CONFIG_CMD_NET) - else if (strcmp(name, "bootfile") == 0) { + else if ((strcmp(name, "bootfile") == 0)&& (newval != NULL)) {copy_filename (BootFile, newval,sizeof (BootFile));return 0 ;}
Quoted 3 lines
Best regards,
Stefano
Best regards,
Gerlando
PATCH 1/1 env: fix crash using default -f -a
Quoted 5 lines · Gerlando Falauto
My idea here was to "apply" the changes as soon as the environment gets changed, like it would happen with a manual "setenv"/"env set". So for instance, if you switch from a custom baudrate back to the default baudrate, you get a message prompting you to switch to the new baudrate (which actually *DOES* change on the console).
This is exactly the reason I am personally not so convinced to do it. This forbids to reset the environment in a script. In fact, nobody wants that a script stops waiting for input.
Think this use case. A board come back from field and in the factory the original status should be restored. It can be easy done with a script that contains "env default -f -a", plus parts to restore the original software. The whole script should run, and not wait for input.
My personal opinion is that who issues such commands (as restoring the environment, or destroying data in flash, or..) knows what is doing and it should not be forbidden to do what he minds.
Quoted 3 lines
Whether that should also be performed as a "cleanup" action upon restoring defaults, well I am not sure...
I explained my opinion above ;-). But this has nothing to do with the patch, that must fix a crash.
Quoted 13 lines
Quoted 6 lines
Quoted 2 lines
So by adding these extra checks you're subtly canceling those efforts.
That is correct, but in any case a check for a NULL pointer should be done before accessing to the pointer, independently what we want to do with it.
Absolutely. See my proposed patch.
Quoted 4 lines
Quoted 3 lines
So I would rather provide a default value to prevent the crashes, instead of skipping settings altogether. Posting a patch in a few minutes.
Quoted 6 lines
Quoted 1 line
Ok, I will review it. In any case, we need a fix for the coming release.
Absolutely. Once again, I cannot apologize and thank you enough for reporting and digging into this.
I assumed providing an own patch would make more sense than asking you to review yours, not sure whether it is popular practice to do so.
Never mind. It is only important to fix the bug ;-)
Best regards,
Stefano
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================