| Message ID | 20201028182820.141279-1-sava.jakovljev@teufel.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | core: Fix handling of upgrade state | expand |
Hi Sava, On 28.10.20 19:28, Sava Jakovljev wrote: > * get_state_string does a dangerous cast to char* which can > later be derefenced, which can lead to SIGSEG - thus return NULL > and make the caller check. > Please show which is the use case causing SEGV - I cannot follow. Where is referenced causing the issue ? > Signed-off-by: Sava Jakovljev <sava.jakovljev@teufel.de> > --- > core/state.c | 3 ++- > include/state.h | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/core/state.c b/core/state.c > index e1ce5fe..4302e1e 100644 > --- a/core/state.c > +++ b/core/state.c > @@ -60,7 +60,8 @@ server_op_res_t save_state(char *key, update_state_t value) > > server_op_res_t save_state_string(char *key, update_state_t value) > { > - return do_save_state(key, get_state_string(value)); > + char* placeholder = get_state_string(value); > + return do_save_state(key, placeholder == NULL ? &value : placeholder); I do not like this - it looks like that issue is somewhere else, and fix can solve your problem but at the wrong place. save_state checks for pointer, so I do not understand the point. Best regards, Stefano Babic > } > > server_op_res_t read_state(char *key, update_state_t *value) > diff --git a/include/state.h b/include/state.h > index e6c6cfa..abdd53f 100644 > --- a/include/state.h > +++ b/include/state.h > @@ -54,7 +54,7 @@ static inline char* get_state_string(update_state_t state) { > case STATE_FAILED: return (char*)"failed"; > default: break; > } > - return (char*)state; > + return NULL; > } > > server_op_res_t save_state(char *key, update_state_t value); >
Hello Stefano, Maybe I was not precise enough - this implementation can cause errors and I see it as a bad practice. In the current master it possibly works, but current master anyway has other errors, and solving them, I encountered this implementation. No need to talk about that those other problems. As far as I see, this implementation is unsafe. Taking an enum and casting it to a char*, and returning with a function which you would expect to return a meaningful string, and definitely not what it does right now. This can also be bad: for example, function do_save_state de-refenreces the value, which is a char* - and function save_state_string calls it in the following way: do_save_state(key, get_state_string(value)); If value is anything but STATE_IN_PROGRESS and STATE_FAILED, segmentation fault can happen - and it did for me - maybe you cannot get it in the current master, but that doesn't mean this implementation isn't unsafe. I would argue to make this code cleaner. get_state_string should always return something meaningful and not do any casts. If the intention was to save the enum value, it should be done differently. Cheers, Sava Jakovljev Stefano Babic schrieb am Mittwoch, 28. Oktober 2020 um 20:45:45 UTC+1: > Hi Sava, > > On 28.10.20 19:28, Sava Jakovljev wrote: > > * get_state_string does a dangerous cast to char* which can > > later be derefenced, which can lead to SIGSEG - thus return NULL > > and make the caller check. > > > > Please show which is the use case causing SEGV - I cannot follow. Where > is referenced causing the issue ? > > > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de> > > --- > > core/state.c | 3 ++- > > include/state.h | 2 +- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/core/state.c b/core/state.c > > index e1ce5fe..4302e1e 100644 > > --- a/core/state.c > > +++ b/core/state.c > > @@ -60,7 +60,8 @@ server_op_res_t save_state(char *key, update_state_t > value) > > > > server_op_res_t save_state_string(char *key, update_state_t value) > > { > > - return do_save_state(key, get_state_string(value)); > > + char* placeholder = get_state_string(value); > > + return do_save_state(key, placeholder == NULL ? &value : placeholder); > > I do not like this - it looks like that issue is somewhere else, and fix > can solve your problem but at the wrong place. save_state checks for > pointer, so I do not understand the point. > > Best regards, > Stefano Babic > > > } > > > > server_op_res_t read_state(char *key, update_state_t *value) > > diff --git a/include/state.h b/include/state.h > > index e6c6cfa..abdd53f 100644 > > --- a/include/state.h > > +++ b/include/state.h > > @@ -54,7 +54,7 @@ static inline char* get_state_string(update_state_t > state) { > > case STATE_FAILED: return (char*)"failed"; > > default: break; > > } > > - return (char*)state; > > + return NULL; > > } > > > > server_op_res_t save_state(char *key, update_state_t value); > > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80 > <+49%208142%206698980> Email: sba...@denx.de > ===================================================================== >
Hi Sava, On 29.10.20 13:11, Sava Jakovljev wrote: > Hello Stefano, > > Maybe I was not precise enough - this implementation can cause errors > and I see it as a bad practice. > In the current master it possibly works, but current master anyway has > other errors, and solving them, I encountered this implementation. > Added Christian in CC, he's the original author for this code. > No need to talk about that those other problems. > As far as I see, this implementation is unsafe. Taking an enum and > casting it to a char*, and returning with a function which you would > expect to return a meaningful string, and definitely not what it does > right now. I can admit this is tricky (but I will let Christian to explain better). Nevertheless until the state is validated to be inside the range, it should not break. You are talking you experienced a SEGV. > This can also be bad: for example, function do_save_state de-refenreces > the value, which is a char* - and function save_state_string calls it in > the following way: > do_save_state(key, get_state_string(value)); > If value is anything but STATE_IN_PROGRESS and STATE_FAILED, ok - that's the point. state should not be set on different values. save_state_string() is also unused (with the exception of marker) in code, should we instead drop this function at all ? > segmentation fault can happen - and it did for me This is just the point (independently from the patch) that I am trying to understand: function is not exported and not part of API, it is just used by SWUpdate, under which conditions does SEGV happen ? > - maybe you cannot get > it in the current master, but that doesn't mean this implementation > isn't unsafe. > > I would argue to make this code cleaner. get_state_string should always > return something meaningful and not do any casts. If the intention was > to save the enum value, it should be done differently. The functionality for the state machine is in save_state() and read_state(), get_state_string() / save_state_string() look quite out of context, maybe both dropped and replaced in the single call with a generic bootloade_env_set() ? Best regards, Stefano Babic > > Cheers, > Sava Jakovljev > > Stefano Babic schrieb am Mittwoch, 28. Oktober 2020 um 20:45:45 UTC+1: > > Hi Sava, > > On 28.10.20 19:28, Sava Jakovljev wrote: > > * get_state_string does a dangerous cast to char* which can > > later be derefenced, which can lead to SIGSEG - thus return NULL > > and make the caller check. > > > > Please show which is the use case causing SEGV - I cannot follow. Where > is referenced causing the issue ? > > > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de> > > --- > > core/state.c | 3 ++- > > include/state.h | 2 +- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/core/state.c b/core/state.c > > index e1ce5fe..4302e1e 100644 > > --- a/core/state.c > > +++ b/core/state.c > > @@ -60,7 +60,8 @@ server_op_res_t save_state(char *key, > update_state_t value) > > > > server_op_res_t save_state_string(char *key, update_state_t value) > > { > > - return do_save_state(key, get_state_string(value)); > > + char* placeholder = get_state_string(value); > > + return do_save_state(key, placeholder == NULL ? &value : > placeholder); > > I do not like this - it looks like that issue is somewhere else, and > fix > can solve your problem but at the wrong place. save_state checks for > pointer, so I do not understand the point. > > Best regards, > Stefano Babic > > > } > > > > server_op_res_t read_state(char *key, update_state_t *value) > > diff --git a/include/state.h b/include/state.h > > index e6c6cfa..abdd53f 100644 > > --- a/include/state.h > > +++ b/include/state.h > > @@ -54,7 +54,7 @@ static inline char* > get_state_string(update_state_t state) { > > case STATE_FAILED: return (char*)"failed"; > > default: break; > > } > > - return (char*)state; > > + return NULL; > > } > > > > server_op_res_t save_state(char *key, update_state_t value); > > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax: > +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de > ===================================================================== > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/102c6f05-bfb6-48e3-a124-5aeff15c85edn%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/102c6f05-bfb6-48e3-a124-5aeff15c85edn%40googlegroups.com?utm_medium=email&utm_source=footer>.
Hello Stefano, If state should not be set than anything else but STATE_IN_PROGRESS and STATE_FAILED, I don't see the point of function doing that cast to char* and returning it, regardless of the way it should be called. I have a feeling it should be made cleaner. What is the point of having return (char*)state? Even if the enum is in range (absolutely doesn't matter), it should not be casted to char*, regardless of everything else. Also, when looking at the code, I don't see the requirement you said: save_state_strings take update_state_t as an argument and calls do_save_state which uses it, thinking that it got a valid char*. I'm not saying that's necessarily an error, but it would be nice if it would be more obvious and cleaner - I admit my patch is maybe a bit dirty, but it began a discussion, and that's always a good thing :) I like the idea of using plain and simple bootloader_env_set and dropping save_state_string if it is not used. To explain the part with SEGV, I don't think it is happening regularly and in current implementation: not to get into too many details, there is a deadlock in activation IPC implementation of Suricatta, because save_state is waiting in read() - and I replaced that call with save_state_string, expecting it would simply work, as name of the function implies - was I wrong in assuming that that function could be used there (if we forget about possible synchronization issues and the motivation to manipulate state only from one process)? Please accept my apologizes if I was too vague. Long story short, I would advocate in making that piece of code better or removing it. Thank you. Cheers, Sava Jakovljev Stefano Babic schrieb am Donnerstag, 29. Oktober 2020 um 17:41:07 UTC+1: > Hi Sava, > > On 29.10.20 13:11, Sava Jakovljev wrote: > > Hello Stefano, > > > > Maybe I was not precise enough - this implementation can cause errors > > and I see it as a bad practice. > > In the current master it possibly works, but current master anyway has > > other errors, and solving them, I encountered this implementation. > > > > Added Christian in CC, he's the original author for this code. > > > No need to talk about that those other problems. > > As far as I see, this implementation is unsafe. Taking an enum and > > casting it to a char*, and returning with a function which you would > > expect to return a meaningful string, and definitely not what it does > > right now. > > I can admit this is tricky (but I will let Christian to explain better). > > Nevertheless until the state is validated to be inside the range, it > should not break. You are talking you experienced a SEGV. > > > This can also be bad: for example, function do_save_state de-refenreces > > the value, which is a char* - and function save_state_string calls it in > > the following way: > > do_save_state(key, get_state_string(value)); > > If value is anything but STATE_IN_PROGRESS and STATE_FAILED, > > ok - that's the point. state should not be set on different values. > > save_state_string() is also unused (with the exception of marker) in > code, should we instead drop this function at all ? > > > segmentation fault can happen - and it did for me > > This is just the point (independently from the patch) that I am trying > to understand: function is not exported and not part of API, it is just > used by SWUpdate, under which conditions does SEGV happen ? > > > - maybe you cannot get > > it in the current master, but that doesn't mean this implementation > > isn't unsafe. > > > > I would argue to make this code cleaner. get_state_string should always > > return something meaningful and not do any casts. If the intention was > > to save the enum value, it should be done differently. > > > The functionality for the state machine is in save_state() and > read_state(), get_state_string() / save_state_string() look quite out of > context, maybe both dropped and replaced in the single call with a > generic bootloade_env_set() ? > > Best regards, > Stefano Babic > > > > > Cheers, > > Sava Jakovljev > > > > Stefano Babic schrieb am Mittwoch, 28. Oktober 2020 um 20:45:45 UTC+1: > > > > Hi Sava, > > > > On 28.10.20 19:28, Sava Jakovljev wrote: > > > * get_state_string does a dangerous cast to char* which can > > > later be derefenced, which can lead to SIGSEG - thus return NULL > > > and make the caller check. > > > > > > > Please show which is the use case causing SEGV - I cannot follow. Where > > is referenced causing the issue ? > > > > > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de> > > > --- > > > core/state.c | 3 ++- > > > include/state.h | 2 +- > > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/core/state.c b/core/state.c > > > index e1ce5fe..4302e1e 100644 > > > --- a/core/state.c > > > +++ b/core/state.c > > > @@ -60,7 +60,8 @@ server_op_res_t save_state(char *key, > > update_state_t value) > > > > > > server_op_res_t save_state_string(char *key, update_state_t value) > > > { > > > - return do_save_state(key, get_state_string(value)); > > > + char* placeholder = get_state_string(value); > > > + return do_save_state(key, placeholder == NULL ? &value : > > placeholder); > > > > I do not like this - it looks like that issue is somewhere else, and > > fix > > can solve your problem but at the wrong place. save_state checks for > > pointer, so I do not understand the point. > > > > Best regards, > > Stefano Babic > > > > > } > > > > > > server_op_res_t read_state(char *key, update_state_t *value) > > > diff --git a/include/state.h b/include/state.h > > > index e6c6cfa..abdd53f 100644 > > > --- a/include/state.h > > > +++ b/include/state.h > > > @@ -54,7 +54,7 @@ static inline char* > > get_state_string(update_state_t state) { > > > case STATE_FAILED: return (char*)"failed"; > > > default: break; > > > } > > > - return (char*)state; > > > + return NULL; > > > } > > > > > > server_op_res_t save_state(char *key, update_state_t value); > > > > > > > -- > > ===================================================================== > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > > Phone: +49-8142-66989-53 <+49%208142%206698953> > <tel:+49%208142%206698953> Fax: > > +49-8142-66989-80 <+49%208142%206698980> <tel:+49%208142%206698980> > Email: sba...@denx.de > > ===================================================================== > > > > -- > > You received this message because you are subscribed to the Google > > Groups "swupdate" group. > > To unsubscribe from this group and stop receiving emails from it, send > > an email to swupdate+u...@googlegroups.com > > <mailto:swupdate+u...@googlegroups.com>. > > To view this discussion on the web visit > > > https://groups.google.com/d/msgid/swupdate/102c6f05-bfb6-48e3-a124-5aeff15c85edn%40googlegroups.com > > < > https://groups.google.com/d/msgid/swupdate/102c6f05-bfb6-48e3-a124-5aeff15c85edn%40googlegroups.com?utm_medium=email&utm_source=footer > >. > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80 > <+49%208142%206698980> Email: sba...@denx.de > ===================================================================== >
Hi Sava, On 29.10.20 18:28, Sava Jakovljev wrote: > Hello Stefano, > > If state should not be set than anything else but STATE_IN_PROGRESS > and STATE_FAILED, I don't see the point of function doing that cast to > char* and returning it, regardless of the way it should be called. > I have a feeling it should be made cleaner. What is the point of having > return (char*)state? Even if the enum is in range (absolutely doesn't > matter), it should not be casted to char*, regardless of everything else. > I went further - if this function cast forward and backward an enum to get a string, and then it is a wrapper for bootenv_set(), we can simply drop it. If we need in future an abstraction that it is not used now, we can introduce it in a better way again. > Also, when looking at the code, I don't see the requirement you said: > save_state_strings take update_state_t as an argument and calls > do_save_state which uses it, thinking that it got a valid char*. > I'm not saying that's necessarily an error, but it would be nice if it > would be more obvious and cleaner - I admit my patch is maybe a bit > dirty, but it began a discussion, and that's always a good thing :) > > I like the idea of using plain and simple bootloader_env_set and > dropping save_state_string if it is not used. This is my proposal. > To explain the part with > SEGV, I don't think it is happening regularly and in current > implementation: not to get into too many details, there is a deadlock in > activation IPC implementation of Suricatta, ok - this is then a different issue and of course should be fixed. > because save_state is > waiting in read() - and I replaced that call with save_state_string, > expecting it would simply work, as name of the function implies - was I > wrong in assuming that that function could be used there (if we forget > about possible synchronization issues and the motivation to manipulate > state only from one process)? Changes are done so that the installer is the owner of the state and other processes should ask the installer. > Please accept my apologizes if I was too vague. > > Long story short, I would advocate in making that piece of code better > or removing it. I vote for removing it. Best regards, Stefano Babic > > Thank you. > Cheers, > Sava Jakovljev > > Stefano Babic schrieb am Donnerstag, 29. Oktober 2020 um 17:41:07 UTC+1: > > Hi Sava, > > On 29.10.20 13:11, Sava Jakovljev wrote: > > Hello Stefano, > > > > Maybe I was not precise enough - this implementation can cause errors > > and I see it as a bad practice. > > In the current master it possibly works, but current master anyway > has > > other errors, and solving them, I encountered this implementation. > > > > Added Christian in CC, he's the original author for this code. > > > No need to talk about that those other problems. > > As far as I see, this implementation is unsafe. Taking an enum and > > casting it to a char*, and returning with a function which you would > > expect to return a meaningful string, and definitely not what it does > > right now. > > I can admit this is tricky (but I will let Christian to explain > better). > > Nevertheless until the state is validated to be inside the range, it > should not break. You are talking you experienced a SEGV. > > > This can also be bad: for example, function do_save_state > de-refenreces > > the value, which is a char* - and function save_state_string calls > it in > > the following way: > > do_save_state(key, get_state_string(value)); > > If value is anything but STATE_IN_PROGRESS and STATE_FAILED, > > ok - that's the point. state should not be set on different values. > > save_state_string() is also unused (with the exception of marker) in > code, should we instead drop this function at all ? > > > segmentation fault can happen - and it did for me > > This is just the point (independently from the patch) that I am trying > to understand: function is not exported and not part of API, it is just > used by SWUpdate, under which conditions does SEGV happen ? > > > - maybe you cannot get > > it in the current master, but that doesn't mean this implementation > > isn't unsafe. > > > > I would argue to make this code cleaner. get_state_string should > always > > return something meaningful and not do any casts. If the intention > was > > to save the enum value, it should be done differently. > > > The functionality for the state machine is in save_state() and > read_state(), get_state_string() / save_state_string() look quite > out of > context, maybe both dropped and replaced in the single call with a > generic bootloade_env_set() ? > > Best regards, > Stefano Babic > > > > > Cheers, > > Sava Jakovljev > > > > Stefano Babic schrieb am Mittwoch, 28. Oktober 2020 um 20:45:45 > UTC+1: > > > > Hi Sava, > > > > On 28.10.20 19:28, Sava Jakovljev wrote: > > > * get_state_string does a dangerous cast to char* which can > > > later be derefenced, which can lead to SIGSEG - thus return NULL > > > and make the caller check. > > > > > > > Please show which is the use case causing SEGV - I cannot follow. > Where > > is referenced causing the issue ? > > > > > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de> > > > --- > > > core/state.c | 3 ++- > > > include/state.h | 2 +- > > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/core/state.c b/core/state.c > > > index e1ce5fe..4302e1e 100644 > > > --- a/core/state.c > > > +++ b/core/state.c > > > @@ -60,7 +60,8 @@ server_op_res_t save_state(char *key, > > update_state_t value) > > > > > > server_op_res_t save_state_string(char *key, update_state_t value) > > > { > > > - return do_save_state(key, get_state_string(value)); > > > + char* placeholder = get_state_string(value); > > > + return do_save_state(key, placeholder == NULL ? &value : > > placeholder); > > > > I do not like this - it looks like that issue is somewhere else, and > > fix > > can solve your problem but at the wrong place. save_state checks for > > pointer, so I do not understand the point. > > > > Best regards, > > Stefano Babic > > > > > } > > > > > > server_op_res_t read_state(char *key, update_state_t *value) > > > diff --git a/include/state.h b/include/state.h > > > index e6c6cfa..abdd53f 100644 > > > --- a/include/state.h > > > +++ b/include/state.h > > > @@ -54,7 +54,7 @@ static inline char* > > get_state_string(update_state_t state) { > > > case STATE_FAILED: return (char*)"failed"; > > > default: break; > > > } > > > - return (char*)state; > > > + return NULL; > > > } > > > > > > server_op_res_t save_state(char *key, update_state_t value); > > > > > > > -- > > ===================================================================== > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> > <tel:+49%208142%206698953> Fax: > > +49-8142-66989-80 <tel:+49%208142%206698980> > <tel:+49%208142%206698980> Email: sba...@denx.de > > ===================================================================== > > > > -- > > You received this message because you are subscribed to the Google > > Groups "swupdate" group. > > To unsubscribe from this group and stop receiving emails from it, > send > > an email to swupdate+u...@googlegroups.com > > <mailto:swupdate+u...@googlegroups.com>. > > To view this discussion on the web visit > > > https://groups.google.com/d/msgid/swupdate/102c6f05-bfb6-48e3-a124-5aeff15c85edn%40googlegroups.com > > > > <https://groups.google.com/d/msgid/swupdate/102c6f05-bfb6-48e3-a124-5aeff15c85edn%40googlegroups.com?utm_medium=email&utm_source=footer>. > > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax: > +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de > ===================================================================== > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/4fdbe8ee-76ba-4f89-a6f8-d7b54864db5dn%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/4fdbe8ee-76ba-4f89-a6f8-d7b54864db5dn%40googlegroups.com?utm_medium=email&utm_source=footer>.
Hi, > If state should not be set than anything else but STATE_IN_PROGRESS > and STATE_FAILED, This was introduced in commit 15dd8d9 with message: "Consolidate transaction marker handling that directly uses the bootloader interface to use the state handling functions instead. Then, persistent state in the bootloader environment is solely managed via state's functions." So, for consolidation purposes, it's supposed to handle just that, save_state() to a bootloader environment's key matching BOOTVAR_TRANSACTION with (translated) values STATE_IN_PROGRESS and STATE_FAILED to maintain backwards compatibility. Not more, not less. The hacky char cast is a catch-all for writing something "sensible" in case of failure ― failure here is not really expected though :) > I don't see the point of function doing that cast to > char* and returning it, regardless of the way it should be called. > I have a feeling it should be made cleaner. What is the point of having > return (char*)state? Even if the enum is in range (absolutely doesn't > matter), it should not be casted to char*, regardless of everything else. > > Also, when looking at the code, I don't see the requirement you said: > save_state_strings take update_state_t as an argument and calls > do_save_state which uses it, thinking that it got a valid char*. > I'm not saying that's necessarily an error, but it would be nice if it > would be more obvious and cleaner - I admit my patch is maybe a bit dirty, > but it began a discussion, and that's always a good thing :) > > I like the idea of using plain and simple bootloader_env_set and dropping > save_state_string if it is not used. To explain the part with SEGV, I don't > think it is happening regularly and in current implementation: not to get > into too many details, there is a deadlock in activation IPC implementation > of Suricatta, because save_state is waiting in read() - and I replaced that > call with save_state_string, expecting it would simply work, as name of the > function implies Yes, gotcha :) Granted, you may have to look into the implementation to judge whether that matches your expectation of the functionality suggested by the name. Definitely an area to improve, e.g., by refactoring and (structured inline) documentation ― which is in particular true for "deeply internal" functions such as these. > - was I wrong in assuming that that function could be used > there (if we forget about possible synchronization issues and the > motivation to manipulate state only from one process)? > Please accept my apologizes if I was too vague. > > Long story short, I would advocate in making that piece of code better or > removing it. The question is whether we still need the BOOTVAR_TRANSACTION marker for some purpose? If not, we can simply remove all of its handling. Otherwise, we may generalize and improve the function but it is of dubious use anyway except for the particular purpose of handling the BOOTVAR_TRANSACTION marker. Kind regards, Christian
Hi Christian, On 30.10.20 15:20, Christian Storm wrote: > Hi, > >> If state should not be set than anything else but STATE_IN_PROGRESS >> and STATE_FAILED, > > This was introduced in commit 15dd8d9 with message: > "Consolidate transaction marker handling that directly uses the > bootloader interface to use the state handling functions instead. > Then, persistent state in the bootloader environment is solely > managed via state's functions." > > So, for consolidation purposes, it's supposed to handle just that, > save_state() to a bootloader environment's key matching > BOOTVAR_TRANSACTION with (translated) values STATE_IN_PROGRESS and > STATE_FAILED to maintain backwards compatibility. Not more, not less. > The hacky char cast is a catch-all for writing something "sensible" in > case of failure ― failure here is not really expected though :) > > >> I don't see the point of function doing that cast to >> char* and returning it, regardless of the way it should be called. >> I have a feeling it should be made cleaner. What is the point of having >> return (char*)state? Even if the enum is in range (absolutely doesn't >> matter), it should not be casted to char*, regardless of everything else. >> >> Also, when looking at the code, I don't see the requirement you said: >> save_state_strings take update_state_t as an argument and calls >> do_save_state which uses it, thinking that it got a valid char*. >> I'm not saying that's necessarily an error, but it would be nice if it >> would be more obvious and cleaner - I admit my patch is maybe a bit dirty, >> but it began a discussion, and that's always a good thing :) >> >> I like the idea of using plain and simple bootloader_env_set and dropping >> save_state_string if it is not used. To explain the part with SEGV, I don't >> think it is happening regularly and in current implementation: not to get >> into too many details, there is a deadlock in activation IPC implementation >> of Suricatta, because save_state is waiting in read() - and I replaced that >> call with save_state_string, expecting it would simply work, as name of the >> function implies > > Yes, gotcha :) Granted, you may have to look into the implementation to > judge whether that matches your expectation of the functionality > suggested by the name. Definitely an area to improve, e.g., by > refactoring and (structured inline) documentation ― which is in > particular true for "deeply internal" functions such as these. > > >> - was I wrong in assuming that that function could be used >> there (if we forget about possible synchronization issues and the >> motivation to manipulate state only from one process)? >> Please accept my apologizes if I was too vague. >> >> Long story short, I would advocate in making that piece of code better or >> removing it. > > The question is whether we still need the BOOTVAR_TRANSACTION marker for > some purpose? Yes - the marker is required in case of single-copy. It sets a transaction flag that is evaluated by the bootloader to start SWUpdate as rescue (ramdisk) again until update is successful, else the bootloader could start a partially written software. It can be (and it is) ignored in case of dual-copy. > If not, we can simply remove all of its handling. > Otherwise, we may generalize and improve the function but it is of > dubious use anyway except for the particular purpose of handling the > BOOTVAR_TRANSACTION marker. Right - so I suggest we add a generalisation when we will find useful. Regards, Stefano
Hi Stefano, > >> If state should not be set than anything else but STATE_IN_PROGRESS > >> and STATE_FAILED, > > > > This was introduced in commit 15dd8d9 with message: > > "Consolidate transaction marker handling that directly uses the > > bootloader interface to use the state handling functions instead. > > Then, persistent state in the bootloader environment is solely > > managed via state's functions." > > > > So, for consolidation purposes, it's supposed to handle just that, > > save_state() to a bootloader environment's key matching > > BOOTVAR_TRANSACTION with (translated) values STATE_IN_PROGRESS and > > STATE_FAILED to maintain backwards compatibility. Not more, not less. > > The hacky char cast is a catch-all for writing something "sensible" in > > case of failure ― failure here is not really expected though :) > > > > > >> I don't see the point of function doing that cast to > >> char* and returning it, regardless of the way it should be called. > >> I have a feeling it should be made cleaner. What is the point of having > >> return (char*)state? Even if the enum is in range (absolutely doesn't > >> matter), it should not be casted to char*, regardless of everything else. > >> > >> Also, when looking at the code, I don't see the requirement you said: > >> save_state_strings take update_state_t as an argument and calls > >> do_save_state which uses it, thinking that it got a valid char*. > >> I'm not saying that's necessarily an error, but it would be nice if it > >> would be more obvious and cleaner - I admit my patch is maybe a bit dirty, > >> but it began a discussion, and that's always a good thing :) > >> > >> I like the idea of using plain and simple bootloader_env_set and dropping > >> save_state_string if it is not used. To explain the part with SEGV, I don't > >> think it is happening regularly and in current implementation: not to get > >> into too many details, there is a deadlock in activation IPC implementation > >> of Suricatta, because save_state is waiting in read() - and I replaced that > >> call with save_state_string, expecting it would simply work, as name of the > >> function implies > > > > Yes, gotcha :) Granted, you may have to look into the implementation to > > judge whether that matches your expectation of the functionality > > suggested by the name. Definitely an area to improve, e.g., by > > refactoring and (structured inline) documentation ― which is in > > particular true for "deeply internal" functions such as these. > > > > > >> - was I wrong in assuming that that function could be used > >> there (if we forget about possible synchronization issues and the > >> motivation to manipulate state only from one process)? > >> Please accept my apologizes if I was too vague. > >> > >> Long story short, I would advocate in making that piece of code better or > >> removing it. > > > > The question is whether we still need the BOOTVAR_TRANSACTION marker for > > some purpose? > > Yes - the marker is required in case of single-copy. It sets a > transaction flag that is evaluated by the bootloader to start SWUpdate > as rescue (ramdisk) again until update is successful, else the > bootloader could start a partially written software. > > It can be (and it is) ignored in case of dual-copy. > > > If not, we can simply remove all of its handling. > > Otherwise, we may generalize and improve the function but it is of > > dubious use anyway except for the particular purpose of handling the > > BOOTVAR_TRANSACTION marker. > > Right - so I suggest we add a generalisation when we will find useful. Hm, can't we model the state marker via ustate, e.g., with STATE_FAILED? core/stream_interface.c::570 does set ustate to STATE_INSTALLED. We would need to extend ustate to also capture aborted in-flight transactions (STATE_IN_PROGRESS) and of course need to modify the bootloader making sense of ustate. Then we can get rid of the transaction marker stuff in favor of one, ustate. Kind regards, Christian
On 30.10.20 16:05, Christian Storm wrote: > Hi Stefano, > > >>>> If state should not be set than anything else but STATE_IN_PROGRESS >>>> and STATE_FAILED, >>> >>> This was introduced in commit 15dd8d9 with message: >>> "Consolidate transaction marker handling that directly uses the >>> bootloader interface to use the state handling functions instead. >>> Then, persistent state in the bootloader environment is solely >>> managed via state's functions." >>> >>> So, for consolidation purposes, it's supposed to handle just that, >>> save_state() to a bootloader environment's key matching >>> BOOTVAR_TRANSACTION with (translated) values STATE_IN_PROGRESS and >>> STATE_FAILED to maintain backwards compatibility. Not more, not less. >>> The hacky char cast is a catch-all for writing something "sensible" in >>> case of failure ― failure here is not really expected though :) >>> >>> >>>> I don't see the point of function doing that cast to >>>> char* and returning it, regardless of the way it should be called. >>>> I have a feeling it should be made cleaner. What is the point of having >>>> return (char*)state? Even if the enum is in range (absolutely doesn't >>>> matter), it should not be casted to char*, regardless of everything else. >>>> >>>> Also, when looking at the code, I don't see the requirement you said: >>>> save_state_strings take update_state_t as an argument and calls >>>> do_save_state which uses it, thinking that it got a valid char*. >>>> I'm not saying that's necessarily an error, but it would be nice if it >>>> would be more obvious and cleaner - I admit my patch is maybe a bit dirty, >>>> but it began a discussion, and that's always a good thing :) >>>> >>>> I like the idea of using plain and simple bootloader_env_set and dropping >>>> save_state_string if it is not used. To explain the part with SEGV, I don't >>>> think it is happening regularly and in current implementation: not to get >>>> into too many details, there is a deadlock in activation IPC implementation >>>> of Suricatta, because save_state is waiting in read() - and I replaced that >>>> call with save_state_string, expecting it would simply work, as name of the >>>> function implies >>> >>> Yes, gotcha :) Granted, you may have to look into the implementation to >>> judge whether that matches your expectation of the functionality >>> suggested by the name. Definitely an area to improve, e.g., by >>> refactoring and (structured inline) documentation ― which is in >>> particular true for "deeply internal" functions such as these. >>> >>> >>>> - was I wrong in assuming that that function could be used >>>> there (if we forget about possible synchronization issues and the >>>> motivation to manipulate state only from one process)? >>>> Please accept my apologizes if I was too vague. >>>> >>>> Long story short, I would advocate in making that piece of code better or >>>> removing it. >>> >>> The question is whether we still need the BOOTVAR_TRANSACTION marker for >>> some purpose? >> >> Yes - the marker is required in case of single-copy. It sets a >> transaction flag that is evaluated by the bootloader to start SWUpdate >> as rescue (ramdisk) again until update is successful, else the >> bootloader could start a partially written software. >> >> It can be (and it is) ignored in case of dual-copy. >> >>> If not, we can simply remove all of its handling. >>> Otherwise, we may generalize and improve the function but it is of >>> dubious use anyway except for the particular purpose of handling the >>> BOOTVAR_TRANSACTION marker. >> >> Right - so I suggest we add a generalisation when we will find useful. > > Hm, can't we model the state marker via ustate, e.g., with STATE_FAILED? One thing is this is another breakage. The variable "recovery_status" was used since the early beginning to set the transaction, and there are plenty of projects where this variable is checked by U-Boot. If we change the name of the variable, we need also to update the bootscripts in field for a lot of project. And weird enough, this is something that can be easy underestimated. > core/stream_interface.c::570 does set ustate to STATE_INSTALLED. Right. > We would need to extend ustate to also capture aborted in-flight > transactions (STATE_IN_PROGRESS) and of course need to modify the > bootloader making sense of ustate. Then we can get rid of the > transaction marker stuff in favor of one, ustate. I guess there are many projects running in single-copy mode where "recovery_status" is checked in U-Boot (like bootcmd), and if we change the name I am afraid that it is very difficult to communicate what should be done. I can propose to have a migration, where we also set "ustate" and we will remove recovery_status after some releases. Best regards, Stefano
Hi Stefano, > >>>> If state should not be set than anything else but STATE_IN_PROGRESS > >>>> and STATE_FAILED, > >>> > >>> This was introduced in commit 15dd8d9 with message: > >>> "Consolidate transaction marker handling that directly uses the > >>> bootloader interface to use the state handling functions instead. > >>> Then, persistent state in the bootloader environment is solely > >>> managed via state's functions." > >>> > >>> So, for consolidation purposes, it's supposed to handle just that, > >>> save_state() to a bootloader environment's key matching > >>> BOOTVAR_TRANSACTION with (translated) values STATE_IN_PROGRESS and > >>> STATE_FAILED to maintain backwards compatibility. Not more, not less. > >>> The hacky char cast is a catch-all for writing something "sensible" in > >>> case of failure ― failure here is not really expected though :) > >>> > >>> > >>>> I don't see the point of function doing that cast to > >>>> char* and returning it, regardless of the way it should be called. > >>>> I have a feeling it should be made cleaner. What is the point of having > >>>> return (char*)state? Even if the enum is in range (absolutely doesn't > >>>> matter), it should not be casted to char*, regardless of everything else. > >>>> > >>>> Also, when looking at the code, I don't see the requirement you said: > >>>> save_state_strings take update_state_t as an argument and calls > >>>> do_save_state which uses it, thinking that it got a valid char*. > >>>> I'm not saying that's necessarily an error, but it would be nice if it > >>>> would be more obvious and cleaner - I admit my patch is maybe a bit dirty, > >>>> but it began a discussion, and that's always a good thing :) > >>>> > >>>> I like the idea of using plain and simple bootloader_env_set and dropping > >>>> save_state_string if it is not used. To explain the part with SEGV, I don't > >>>> think it is happening regularly and in current implementation: not to get > >>>> into too many details, there is a deadlock in activation IPC implementation > >>>> of Suricatta, because save_state is waiting in read() - and I replaced that > >>>> call with save_state_string, expecting it would simply work, as name of the > >>>> function implies > >>> > >>> Yes, gotcha :) Granted, you may have to look into the implementation to > >>> judge whether that matches your expectation of the functionality > >>> suggested by the name. Definitely an area to improve, e.g., by > >>> refactoring and (structured inline) documentation ― which is in > >>> particular true for "deeply internal" functions such as these. > >>> > >>> > >>>> - was I wrong in assuming that that function could be used > >>>> there (if we forget about possible synchronization issues and the > >>>> motivation to manipulate state only from one process)? > >>>> Please accept my apologizes if I was too vague. > >>>> > >>>> Long story short, I would advocate in making that piece of code better or > >>>> removing it. > >>> > >>> The question is whether we still need the BOOTVAR_TRANSACTION marker for > >>> some purpose? > >> > >> Yes - the marker is required in case of single-copy. It sets a > >> transaction flag that is evaluated by the bootloader to start SWUpdate > >> as rescue (ramdisk) again until update is successful, else the > >> bootloader could start a partially written software. > >> > >> It can be (and it is) ignored in case of dual-copy. > >> > >>> If not, we can simply remove all of its handling. > >>> Otherwise, we may generalize and improve the function but it is of > >>> dubious use anyway except for the particular purpose of handling the > >>> BOOTVAR_TRANSACTION marker. > >> > >> Right - so I suggest we add a generalisation when we will find useful. > > > > Hm, can't we model the state marker via ustate, e.g., with STATE_FAILED? > > One thing is this is another breakage. The variable "recovery_status" > was used since the early beginning to set the transaction, and there are > plenty of projects where this variable is checked by U-Boot. If we > change the name of the variable, we need also to update the bootscripts > in field for a lot of project. And weird enough, this is something that > can be easy underestimated. Yes, totally agreed. > > core/stream_interface.c::570 does set ustate to STATE_INSTALLED. > > Right. > > > We would need to extend ustate to also capture aborted in-flight > > transactions (STATE_IN_PROGRESS) and of course need to modify the > > bootloader making sense of ustate. Then we can get rid of the > > transaction marker stuff in favor of one, ustate. > > I guess there are many projects running in single-copy mode where > "recovery_status" is checked in U-Boot (like bootcmd), and if we change > the name I am afraid that it is very difficult to communicate what > should be done. I can propose to have a migration, where we also set > "ustate" and we will remove recovery_status after some releases. It's probably not just the name but also the meaning of the variable, but anyway, sounds like a good plan moving forward :) Kind regards, Christian
diff --git a/core/state.c b/core/state.c index e1ce5fe..4302e1e 100644 --- a/core/state.c +++ b/core/state.c @@ -60,7 +60,8 @@ server_op_res_t save_state(char *key, update_state_t value) server_op_res_t save_state_string(char *key, update_state_t value) { - return do_save_state(key, get_state_string(value)); + char* placeholder = get_state_string(value); + return do_save_state(key, placeholder == NULL ? &value : placeholder); } server_op_res_t read_state(char *key, update_state_t *value) diff --git a/include/state.h b/include/state.h index e6c6cfa..abdd53f 100644 --- a/include/state.h +++ b/include/state.h @@ -54,7 +54,7 @@ static inline char* get_state_string(update_state_t state) { case STATE_FAILED: return (char*)"failed"; default: break; } - return (char*)state; + return NULL; } server_op_res_t save_state(char *key, update_state_t value);
* get_state_string does a dangerous cast to char* which can later be derefenced, which can lead to SIGSEG - thus return NULL and make the caller check. Signed-off-by: Sava Jakovljev <sava.jakovljev@teufel.de> --- core/state.c | 3 ++- include/state.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)