| Message ID | 9355fc98e3dac5768ecaf9e179be2f7a0e74d633.1707839454.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 76fb807faacc38661ddb1c561ed80930699146ec |
| Headers | show |
| Series | column: disallow negative padding | expand |
Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > Make sure that client code can’t pass in a negative padding by accident. > > Suggested-by: Rubén Justo <rjusto@gmail.com> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > --- > > Notes (series): > Apparently these are the only publicly-visible functions that use this > struct according to `column.h`. > > column.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/column.c b/column.c > index ff2f0abf399..50bbccc92ee 100644 > --- a/column.c > +++ b/column.c > @@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts, > { > struct column_options nopts; > > + if (opts && (0 > opts->padding)) > + BUG("padding must be non-negative"); The only two current callers may happen to be "git branch" that passes NULL as opts, and "git clean" that passes 2 in opts->padding, so this BUG() will not trigger. Once we add new callers to this function, or update the current callers, this safety start to matter. The actual breakage from a negative padding happens in layout(), so another option would be to have this guard there, which will protect us from having new callers of that function as well, or its caller display_table(), but these have only one caller each, so having the guard print_columns() here, that is the closest to the callers would be fine. > if (!list->nr) > return; > assert((colopts & COL_ENABLE_MASK) != COL_AUTO); > @@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts) > { > struct strvec *argv; > > + if (opts && (0 > opts->padding)) > + BUG("padding must be non-negative"); This one happens to be safe currently because "git tag" passes 2 in opts->padding, but I do not think this is needed. We will pass these through to "git column" and the negative padding will be caught as an error there anyway, no? So whether "git tag" is updated or a new caller of run_column_filter() is added, the developer will already notice it (and they will have to protect themselves just like the [1/2] of your series did for "git column" itself). > if (fd_out != -1) > return -1;
On 13-feb-2024 09:06:46, Junio C Hamano wrote: > Kristoffer Haugsbakk <code@khaugsbakk.name> writes: > > > Make sure that client code can’t pass in a negative padding by accident. > > > > Suggested-by: Rubén Justo <rjusto@gmail.com> > > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > > --- > > > > Notes (series): > > Apparently these are the only publicly-visible functions that use this > > struct according to `column.h`. > > > > column.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/column.c b/column.c > > index ff2f0abf399..50bbccc92ee 100644 > > --- a/column.c > > +++ b/column.c > > @@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts, > > { > > struct column_options nopts; > > > > + if (opts && (0 > opts->padding)) > > + BUG("padding must be non-negative"); > > The only two current callers may happen to be "git branch" that > passes NULL as opts, and "git clean" that passes 2 in opts->padding, > so this BUG() will not trigger. Once we add new callers to this > function, or update the current callers, this safety start to matter. > > The actual breakage from a negative padding happens in layout(), > so another option would be to have this guard there, which will > protect us from having new callers of that function as well, or > its caller display_table(), but these have only one caller each, > so having the guard print_columns() here, that is the closest to > the callers would be fine. > > > if (!list->nr) > > return; > > assert((colopts & COL_ENABLE_MASK) != COL_AUTO); > > @@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts) > > { > > struct strvec *argv; > > > > + if (opts && (0 > opts->padding)) > > + BUG("padding must be non-negative"); > > This one happens to be safe currently because "git tag" passes 2 in > opts->padding, but I do not think this is needed. At first glance, I also thought this was not necessary. However, callers of run_column_filter() might forget to check the return value, and the BUG() triggered by the underlying process could be buried and ignored. Having the BUG() here, in the same process, makes it more noticeable. Based on this, I'm not opposed to this change.
Rubén Justo <rjusto@gmail.com> writes: >> This one happens to be safe currently because "git tag" passes 2 in >> opts->padding, but I do not think this is needed. > > At first glance, I also thought this was not necessary. > > However, callers of run_column_filter() might forget to check the return > value, and the BUG() triggered by the underlying process could be buried > and ignored. Having the BUG() here, in the same process, makes it more > noticeable. The point of BUG() is to help developers catch the silly breakage before it excapes from the lab, and we can expect these careless developers to ignore the return value. But "column --padding=-1" invoked as a subprocess will show a human-readable error message to such a developer, so it is less important than the BUG() in the other place. There is no black or white decision, but this one is much less darker gray than the other one is.
On 13-feb-2024 11:39:11, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > >> This one happens to be safe currently because "git tag" passes 2 in > >> opts->padding, but I do not think this is needed. > > > > At first glance, I also thought this was not necessary. > > > > However, callers of run_column_filter() might forget to check the return > > value, and the BUG() triggered by the underlying process could be buried > > and ignored. Having the BUG() here, in the same process, makes it more > > noticeable. > > The point of BUG() is to help developers catch the silly breakage > before it excapes from the lab, and we can expect these careless > developers to ignore the return value. But "column --padding=-1" > invoked as a subprocess will show a human-readable error message > to such a developer, so it is less important than the BUG() in the > other place. > > There is no black or white decision, but this one is much less > darker gray than the other one is. I've checked this, without that BUG(), and the result has not been pretty: diff --git a/builtin/tag.c b/builtin/tag.c index 37473ac21f..e15dfa73d2 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -529,7 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (column_active(colopts)) { struct column_options copts; memset(&copts, 0, sizeof(copts)); - copts.padding = 2; + copts.padding = -1; run_column_filter(colopts, &copts); } filter.name_patterns = argv; I can imagine a future change that opens that current "2" to the user. And the possible report from a user who tries "-1" would not be easy. But I agree with you, that BUG() does not leave a good taste in the mouth. Maybe we should refactor run_column_filter(), I don't know, but I think that is outside of the scope of this series.
On Tue, Feb 13, 2024, at 20:56, Rubén Justo wrote: > On 13-feb-2024 11:39:11, Junio C Hamano wrote: >> Rubén Justo <rjusto@gmail.com> writes: >> The point of BUG() is to help developers catch the silly breakage >> before it excapes from the lab, and we can expect these careless >> developers to ignore the return value. But "column --padding=-1" >> invoked as a subprocess will show a human-readable error message >> to such a developer, so it is less important than the BUG() in the >> other place. >> >> There is no black or white decision, but this one is much less >> darker gray than the other one is. > > I've checked this, without that BUG(), and the result has not been > pretty: > > diff --git a/builtin/tag.c b/builtin/tag.c > index 37473ac21f..e15dfa73d2 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -529,7 +529,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) > if (column_active(colopts)) { > struct column_options copts; > memset(&copts, 0, sizeof(copts)); > - copts.padding = 2; > + copts.padding = -1; > run_column_filter(colopts, &copts); > } > filter.name_patterns = argv; > > I can imagine a future change that opens that current "2" to the user. > And the possible report from a user who tries "-1" would not be easy. > > But I agree with you, that BUG() does not leave a good taste in the > mouth. > > Maybe we should refactor run_column_filter(), I don't know, but I think > that is outside of the scope of this series. Thanks for trying that out—some very topical testing! I will take the night to think about v4. But I will defer to the reviewers’ judgement on the scope of this series/change. (All I know is that it can be tricky balancing such defensive checks with readability and maintanability.) Thanks
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes: > (All I know is that it can be tricky balancing such defensive checks > with readability and maintanability.) Personally I think v3 is a good enough place to stop and we are now entering into the realm of diminishing returns. Thanks.
On 13-feb-2024 11:39:11, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > >> This one happens to be safe currently because "git tag" passes 2 in > >> opts->padding, but I do not think this is needed. > > > > At first glance, I also thought this was not necessary. > > > > However, callers of run_column_filter() might forget to check the return > > value, and the BUG() triggered by the underlying process could be buried > > and ignored. Having the BUG() here, in the same process, makes it more > > noticeable. > > The point of BUG() is to help developers catch the silly breakage > before it excapes from the lab, and we can expect these careless > developers to ignore the return value. But "column --padding=-1" > invoked as a subprocess will show a human-readable error message > to such a developer, so it is less important than the BUG() in the > other place. Thinking again about this; you are right. That BUG() in run_column_filter() does not make sense in this series. It is addressing a different error, and perhaps a solution could be: --- >8 --- Subject: [PATCH] tag: error when git-column fails If the user asks for the list of tags to be displayed in columns ("--columns"), a child git-column process is used to format the output as expected. In a rare situation where we encounter a problem spawning that child process, we will work erroneously. Make noticeable we're having a problem executing git-column, so the user can act accordingly. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- builtin/tag.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/tag.c b/builtin/tag.c index 37473ac21f..30532b76d5 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -530,7 +530,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix) struct column_options copts; memset(&copts, 0, sizeof(copts)); copts.padding = 2; - run_column_filter(colopts, &copts); + if (run_column_filter(colopts, &copts)) + die(_("could not start 'git column'") } filter.name_patterns = argv; ret = list_tags(&filter, sorting, &format);
diff --git a/column.c b/column.c index ff2f0abf399..50bbccc92ee 100644 --- a/column.c +++ b/column.c @@ -182,6 +182,8 @@ void print_columns(const struct string_list *list, unsigned int colopts, { struct column_options nopts; + if (opts && (0 > opts->padding)) + BUG("padding must be non-negative"); if (!list->nr) return; assert((colopts & COL_ENABLE_MASK) != COL_AUTO); @@ -361,6 +363,8 @@ int run_column_filter(int colopts, const struct column_options *opts) { struct strvec *argv; + if (opts && (0 > opts->padding)) + BUG("padding must be non-negative"); if (fd_out != -1) return -1;
Make sure that client code can’t pass in a negative padding by accident. Suggested-by: Rubén Justo <rjusto@gmail.com> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> --- Notes (series): Apparently these are the only publicly-visible functions that use this struct according to `column.h`. column.c | 4 ++++ 1 file changed, 4 insertions(+)