| Message ID | 20250117074909.1430067-1-mh@glandium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 49b299215dd779b781c5a39af85a5acb6294acbd |
| Headers | show |
| Series | connect: address -Wsign-compare warnings | expand |
On Fri, Jan 17, 2025 at 04:49:09PM +0900, Mike Hommey wrote: > diff --git a/connect.c b/connect.c > index 10fad43e98..91f3990014 100644 > --- a/connect.c > +++ b/connect.c > @@ -77,7 +76,7 @@ static NORETURN void die_initial_contact(int unexpected) > /* Checks if the server supports the capability 'c' */ > int server_supports_v2(const char *c) > { > - int i; > + size_t i; > > for (i = 0; i < server_capabilities_v2.nr; i++) { > const char *out; I know that it's often frowned upon to change formatting while at it. But in the context of these refactorings I think that it's quite helpful if you also moved the loop index variable declarations into the loops themselves. This allows us to trivially see that it's not used anywhere else. > @@ -232,12 +231,12 @@ static void annotate_refs_with_symref_info(struct ref *ref) > string_list_clear(&symref, 0); > } > > -static void process_capabilities(struct packet_reader *reader, int *linelen) > +static void process_capabilities(struct packet_reader *reader, size_t *linelen) > { > const char *feat_val; > size_t feat_len; > const char *line = reader->line; > - int nul_location = strlen(line); > + size_t nul_location = strlen(line); > if (nul_location == *linelen) > return; > server_capabilities_v1 = xstrdup(line + nul_location + 1); I think splitting out the strlen(3p)-related changes into a separate commit might make sense. Thanks for working on this, quite happy to see that this gets picked up by the community! Patrick
Mike Hommey <mh@glandium.org> writes: > Most of the warnings were about loop variables being declared as ints > with a condition using a size_t, whereby switching the variable to > size_t fixes the warning. > > One other case was comparing the result of strlen to an int passed > as an argument, which turns out could just as well be passed as a > size_t, albeit trickling to other functions. As long as the blast radius is limited (like this one, which most of the cascades were within the callchain of file-scope statics), and the changes of type is going in the right direction (in this case, I see all are using size_t for length that may come from or compared with the result of strlen(), which falls into that category), such a change is very much welcomed. Even if the primary objective is to squelch the -Wsign-compare and even if we are talking about a line in packet_reader object, which would not exceed 64k bytes and using size_t is way overkill, that is. I personally do not think -Wsign-compare cleanliness is buying us all that much, compared to the amount of code churn. But this one is well within the level that I can tolerate ;-). Will queue. Thanks.
On Fri, Jan 17, 2025 at 09:26:32AM -0800, Junio C Hamano wrote: > Mike Hommey <mh@glandium.org> writes: > > > Most of the warnings were about loop variables being declared as ints > > with a condition using a size_t, whereby switching the variable to > > size_t fixes the warning. > > > > One other case was comparing the result of strlen to an int passed > > as an argument, which turns out could just as well be passed as a > > size_t, albeit trickling to other functions. > > As long as the blast radius is limited (like this one, which most of > the cascades were within the callchain of file-scope statics), and > the changes of type is going in the right direction (in this case, I > see all are using size_t for length that may come from or compared > with the result of strlen(), which falls into that category), such a > change is very much welcomed. > > Even if the primary objective is to squelch the -Wsign-compare and > even if we are talking about a line in packet_reader object, which > would not exceed 64k bytes and using size_t is way overkill, that > is. I personally do not think -Wsign-compare cleanliness is buying > us all that much, compared to the amount of code churn. But this > one is well within the level that I can tolerate ;-). > > Will queue. Thanks. Do you want me to address Patrick's comments? Mike
Mike Hommey <mh@glandium.org> writes: > On Fri, Jan 17, 2025 at 09:26:32AM -0800, Junio C Hamano wrote: > ... > Do you want me to address Patrick's comments? I'll leave it up to Patrick, who is the primary developer who is driving this effort from my point of view. My "Will queue" is merely to queue it in 'seen' so that it won't be forgotten, and means nothing more than that. Thanks.
On Sat, Jan 18, 2025 at 06:18:30AM +0900, Mike Hommey wrote: > On Fri, Jan 17, 2025 at 09:26:32AM -0800, Junio C Hamano wrote: > > Mike Hommey <mh@glandium.org> writes: > > > > > Most of the warnings were about loop variables being declared as ints > > > with a condition using a size_t, whereby switching the variable to > > > size_t fixes the warning. > > > > > > One other case was comparing the result of strlen to an int passed > > > as an argument, which turns out could just as well be passed as a > > > size_t, albeit trickling to other functions. > > > > As long as the blast radius is limited (like this one, which most of > > the cascades were within the callchain of file-scope statics), and > > the changes of type is going in the right direction (in this case, I > > see all are using size_t for length that may come from or compared > > with the result of strlen(), which falls into that category), such a > > change is very much welcomed. > > > > Even if the primary objective is to squelch the -Wsign-compare and > > even if we are talking about a line in packet_reader object, which > > would not exceed 64k bytes and using size_t is way overkill, that > > is. I personally do not think -Wsign-compare cleanliness is buying > > us all that much, compared to the amount of code churn. But this > > one is well within the level that I can tolerate ;-). It does generate quite a bit of churn indeed. But it also made us look a lot closer in many places where such warnings are generated, and we found multiple sites already where unexpected values can cause us to do weird stuff, including going out of bounds. So if this allows us to detect (or even better avoid introducing) even a single out-of-bounds read/write that can be exploited I'm happy. I think overall it's going to be a net win in the long term as it forces us to think more carefullly about types, which we haven't really been doing until now. And this is a frequent observation during code reviews, so it also gets a tiny fraction of reviewer's time back. > Do you want me to address Patrick's comments? I don't mind it too much, the end result would be the same anyway. Just keep it in mind for future patch series. Patrick
diff --git a/connect.c b/connect.c index 10fad43e98..91f3990014 100644 --- a/connect.c +++ b/connect.c @@ -1,5 +1,4 @@ #define USE_THE_REPOSITORY_VARIABLE -#define DISABLE_SIGN_COMPARE_WARNINGS #include "git-compat-util.h" #include "config.h" @@ -77,7 +76,7 @@ static NORETURN void die_initial_contact(int unexpected) /* Checks if the server supports the capability 'c' */ int server_supports_v2(const char *c) { - int i; + size_t i; for (i = 0; i < server_capabilities_v2.nr; i++) { const char *out; @@ -96,7 +95,7 @@ void ensure_server_supports_v2(const char *c) int server_feature_v2(const char *c, const char **v) { - int i; + size_t i; for (i = 0; i < server_capabilities_v2.nr; i++) { const char *out; @@ -112,7 +111,7 @@ int server_feature_v2(const char *c, const char **v) int server_supports_feature(const char *c, const char *feature, int die_on_error) { - int i; + size_t i; for (i = 0; i < server_capabilities_v2.nr; i++) { const char *out; @@ -232,12 +231,12 @@ static void annotate_refs_with_symref_info(struct ref *ref) string_list_clear(&symref, 0); } -static void process_capabilities(struct packet_reader *reader, int *linelen) +static void process_capabilities(struct packet_reader *reader, size_t *linelen) { const char *feat_val; size_t feat_len; const char *line = reader->line; - int nul_location = strlen(line); + size_t nul_location = strlen(line); if (nul_location == *linelen) return; server_capabilities_v1 = xstrdup(line + nul_location + 1); @@ -271,14 +270,14 @@ static int process_dummy_ref(const struct packet_reader *reader) !strcmp(name, "capabilities^{}"); } -static void check_no_capabilities(const char *line, int len) +static void check_no_capabilities(const char *line, size_t len) { if (strlen(line) != len) warning(_("ignoring capabilities after first line '%s'"), line + strlen(line)); } -static int process_ref(const struct packet_reader *reader, int len, +static int process_ref(const struct packet_reader *reader, size_t len, struct ref ***list, unsigned int flags, struct oid_array *extra_have) { @@ -306,7 +305,7 @@ static int process_ref(const struct packet_reader *reader, int len, return 1; } -static int process_shallow(const struct packet_reader *reader, int len, +static int process_shallow(const struct packet_reader *reader, size_t len, struct oid_array *shallow_points) { const char *line = reader->line; @@ -341,7 +340,7 @@ struct ref **get_remote_heads(struct packet_reader *reader, struct oid_array *shallow_points) { struct ref **orig_list = list; - int len = 0; + size_t len = 0; enum get_remote_heads_state state = EXPECTING_FIRST_REF; *list = NULL; @@ -394,7 +393,7 @@ static int process_ref_v2(struct packet_reader *reader, struct ref ***list, const char **unborn_head_target) { int ret = 1; - int i = 0; + size_t i = 0; struct object_id old_oid; struct ref *ref; struct string_list line_sections = STRING_LIST_INIT_DUP; @@ -552,7 +551,7 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader, const struct string_list *server_options, int stateless_rpc) { - int i; + size_t i; struct strvec *ref_prefixes = transport_options ? &transport_options->ref_prefixes : NULL; const char **unborn_head_target = transport_options ?
Most of the warnings were about loop variables being declared as ints with a condition using a size_t, whereby switching the variable to size_t fixes the warning. One other case was comparing the result of strlen to an int passed as an argument, which turns out could just as well be passed as a size_t, albeit trickling to other functions. Signed-off-by: Mike Hommey <mh@glandium.org> --- connect.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)