| Message ID | 20200712100537.17077-2-toertel@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | swupdate-progress: Terminate bar array string | expand |
Hi Mark, On 12.07.20 12:05, Mark Jonas wrote: > The bar array contains the progress bar. Its length is 60 characters > but space for a terminating \0 was forgotten. This causes problems when > feeding bar to fprintf(). > Ok - so I understand the issue is the missing '\0' when bar reaches 100%. > Suggested-by: Tomas Frydrych <tomas@tomtain.com> > Signed-off-by: Mark Jonas <toertel@gmail.com> > --- > tools/swupdate-progress.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c > index 5a2c888..a96793d 100644 > --- a/tools/swupdate-progress.c > +++ b/tools/swupdate-progress.c > @@ -179,7 +179,8 @@ int main(int argc, char **argv) > int psplash_ok = 0; > unsigned int curstep = 0; > unsigned int percent = 0; > - char bar[60]; > + const int bar_len = 60; > + char bar[bar_len+1]; > unsigned int filled_len; > int opt_c = 0; > int opt_w = 0; > @@ -222,7 +223,7 @@ int main(int argc, char **argv) > break; > } > } > - > + > rundir = getenv("PSPLASH_FIFO_DIR"); > if (!rundir) > rundir = "/run"; > @@ -292,12 +293,13 @@ int main(int argc, char **argv) > if ((msg.cur_step != curstep) && (curstep != 0)) > fprintf(stdout, "\n"); > > - filled_len = sizeof(bar) * msg.cur_percent / 100; > - if (filled_len > sizeof(bar) - 1) > - filled_len = sizeof(bar) - 1; > + filled_len = bar_len * msg.cur_percent / 100; > + if (filled_len > bar_len) > + filled_len = bar_len; Anyway, current code is writing at maximu 59 chars instead of 60. You are just adding a further byte, that is line is just a byte longer. But according to the issue, is it not enough to write once a trailing '\0' into the string, or better to initialize the bar array before the while() loop ? The current code should never write into the last byte of the bar array. > > memset(bar,'=', filled_len); > - memset(&bar[filled_len], '-', sizeof(bar) - filled_len - 1); > + memset(&bar[filled_len], '-', bar_len - filled_len); This does not change the behavior. > + bar[bar_len] = '\0'; This is what is missing and fixes the issue IMHO, that it is enough to terminate the string. > > fprintf(stdout, "[ %.60s ] %d of %d %d%% (%s)\r", > bar, > Best regards, Stefano
Hi Stefano, > On 12.07.20 12:05, Mark Jonas wrote: > > The bar array contains the progress bar. Its length is 60 characters > > but space for a terminating \0 was forgotten. This causes problems when > > feeding bar to fprintf(). > > > > Ok - so I understand the issue is the missing '\0' when bar reaches 100%. I think the '\0' at the end of the bar array is always missing. > > - filled_len = sizeof(bar) * msg.cur_percent / 100; > > - if (filled_len > sizeof(bar) - 1) > > - filled_len = sizeof(bar) - 1; > > + filled_len = bar_len * msg.cur_percent / 100; > > + if (filled_len > bar_len) > > + filled_len = bar_len; > > Anyway, current code is writing at maximu 59 chars instead of 60. You > are just adding a further byte, that is line is just a byte longer. But > according to the issue, is it not enough to write once a trailing '\0' > into the string, or better to initialize the bar array before the > while() loop ? The current code should never write into the last byte of > the bar array. My understanding from the code was that the intention was that the bar had a length of 60 visible characters. I considered the limit to 59 visible characters a bug of the code. If you say that it was always the intention to have a bar of 59 visible characters that is fine. > > > > memset(bar,'=', filled_len); > > - memset(&bar[filled_len], '-', sizeof(bar) - filled_len - 1); > > + memset(&bar[filled_len], '-', bar_len - filled_len); > > This does not change the behavior. Yes, that was the intention. But the bar now has 60 visible characters. > > + bar[bar_len] = '\0'; > > This is what is missing and fixes the issue IMHO, that it is enough to > terminate the string. Correct. If you want to keep a visible bar length of 59 characters only the termination is missing. Shall I send an updated patch which keeps a visible bar length of 59 characters? I would prefer to always write the '\0' the after updating the bar to make it easy for humans to understand that the bar[] is properly terminated. Greetings, Mark
Hi Mark, On 12.07.20 12:49, Mark Jonas wrote: > Hi Stefano, > >> On 12.07.20 12:05, Mark Jonas wrote: >>> The bar array contains the progress bar. Its length is 60 characters >>> but space for a terminating \0 was forgotten. This causes problems when >>> feeding bar to fprintf(). >>> >> >> Ok - so I understand the issue is the missing '\0' when bar reaches 100%. > > I think the '\0' at the end of the bar array is always missing. > Agree, this is a bug, and bar was never initialized. >>> - filled_len = sizeof(bar) * msg.cur_percent / 100; >>> - if (filled_len > sizeof(bar) - 1) >>> - filled_len = sizeof(bar) - 1; >>> + filled_len = bar_len * msg.cur_percent / 100; >>> + if (filled_len > bar_len) >>> + filled_len = bar_len; >> >> Anyway, current code is writing at maximu 59 chars instead of 60. You >> are just adding a further byte, that is line is just a byte longer. But >> according to the issue, is it not enough to write once a trailing '\0' >> into the string, or better to initialize the bar array before the >> while() loop ? The current code should never write into the last byte of >> the bar array. > > My understanding from the code was that the intention was that the bar > had a length of 60 visible characters. I considered the limit to 59 visible > characters a bug of the code. If you say that it was always the intention > to have a bar of 59 visible characters that is fine. I just want to understand the issue - the length of the bar is not important. I cannot say why I choose 60, it maybe fits very well in a 80-columns terminal. It was reduced to 59 to previous bugs, it is ok to set it to 60. > >>> >>> memset(bar,'=', filled_len); >>> - memset(&bar[filled_len], '-', sizeof(bar) - filled_len - 1); >>> + memset(&bar[filled_len], '-', bar_len - filled_len); >> >> This does not change the behavior. > > Yes, that was the intention. But the bar now has 60 visible characters. > Ok, got it. >>> + bar[bar_len] = '\0'; >> >> This is what is missing and fixes the issue IMHO, that it is enough to >> terminate the string. > > Correct. If you want to keep a visible bar length of 59 characters only the > termination is missing. > > Shall I send an updated patch which keeps a visible bar length of 59 > characters? > No, my intention was just to understand the issue and how it is fixed. I am fine with the patch. > I would prefer to always write the '\0' the after updating the bar to make > it easy for humans to understand that the bar[] is properly terminated. Agree. Regards, Stefano > > Greetings, > Mark >
Hi Stefano, > >> Ok - so I understand the issue is the missing '\0' when bar reaches 100%. > > > > I think the '\0' at the end of the bar array is always missing. > > > > Agree, this is a bug, and bar was never initialized. > > >>> - filled_len = sizeof(bar) * msg.cur_percent / 100; > >>> - if (filled_len > sizeof(bar) - 1) > >>> - filled_len = sizeof(bar) - 1; > >>> + filled_len = bar_len * msg.cur_percent / 100; > >>> + if (filled_len > bar_len) > >>> + filled_len = bar_len; > >> > >> Anyway, current code is writing at maximu 59 chars instead of 60. You > >> are just adding a further byte, that is line is just a byte longer. But > >> according to the issue, is it not enough to write once a trailing '\0' > >> into the string, or better to initialize the bar array before the > >> while() loop ? The current code should never write into the last byte of > >> the bar array. > > > > My understanding from the code was that the intention was that the bar > > had a length of 60 visible characters. I considered the limit to 59 visible > > characters a bug of the code. If you say that it was always the intention > > to have a bar of 59 visible characters that is fine. > > I just want to understand the issue - the length of the bar is not > important. I cannot say why I choose 60, it maybe fits very well in a > 80-columns terminal. It was reduced to 59 to previous bugs, it is ok to > set it to 60. Sorry, it seems I forgot to properly explain the motivation. When looking at the code I saw that the bar was printed using the format string "[ %.60s ]". From that I derived that the length of the bar should be 60 visible characters. Thus char bar[60] was too small. During further analysis of the code it became hard for me to track the physical length of the bar as sizeof(bar)-1. So I decided to introduce the constant bar_len=60. Right now after writing the explanation above and looking through my patch I realized that there is a tiny flaw: The bar length is contained in the constant bar_len as well as hard coded into the format string. I will fix this in an update of the patch. fprintf(stdout, "[ %.60s ] %d of %d %d%% (%s)\r", bar, msg.cur_step, msg.nsteps, msg.cur_percent, msg.cur_image); will become fprintf(stdout, "[ %.*s ] %d of %d %d%% (%s)\r", bar_len, bar, msg.cur_step, msg.nsteps, msg.cur_percent, msg.cur_image); Now it should be sufficient to change the constant bar_len to an arbitrary value to change the physical length of the progress bar. Greetings, Mark
diff --git a/tools/swupdate-progress.c b/tools/swupdate-progress.c index 5a2c888..a96793d 100644 --- a/tools/swupdate-progress.c +++ b/tools/swupdate-progress.c @@ -179,7 +179,8 @@ int main(int argc, char **argv) int psplash_ok = 0; unsigned int curstep = 0; unsigned int percent = 0; - char bar[60]; + const int bar_len = 60; + char bar[bar_len+1]; unsigned int filled_len; int opt_c = 0; int opt_w = 0; @@ -222,7 +223,7 @@ int main(int argc, char **argv) break; } } - + rundir = getenv("PSPLASH_FIFO_DIR"); if (!rundir) rundir = "/run"; @@ -292,12 +293,13 @@ int main(int argc, char **argv) if ((msg.cur_step != curstep) && (curstep != 0)) fprintf(stdout, "\n"); - filled_len = sizeof(bar) * msg.cur_percent / 100; - if (filled_len > sizeof(bar) - 1) - filled_len = sizeof(bar) - 1; + filled_len = bar_len * msg.cur_percent / 100; + if (filled_len > bar_len) + filled_len = bar_len; memset(bar,'=', filled_len); - memset(&bar[filled_len], '-', sizeof(bar) - filled_len - 1); + memset(&bar[filled_len], '-', bar_len - filled_len); + bar[bar_len] = '\0'; fprintf(stdout, "[ %.60s ] %d of %d %d%% (%s)\r", bar,
The bar array contains the progress bar. Its length is 60 characters but space for a terminating \0 was forgotten. This causes problems when feeding bar to fprintf(). Suggested-by: Tomas Frydrych <tomas@tomtain.com> Signed-off-by: Mark Jonas <toertel@gmail.com> --- tools/swupdate-progress.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)