Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix stringop-overflow warning of GCC #201

Merged
merged 1 commit into from Jun 23, 2018
Merged

fix stringop-overflow warning of GCC #201

merged 1 commit into from Jun 23, 2018

Conversation

@Iniesta8
Copy link
Contributor

@Iniesta8 Iniesta8 commented Jun 12, 2018

When using a modern GCC to compile c-ares, there is a stringop-overflow warning as follows:

c-ares/ares_parse_ptr_reply.c: In function ‘ares_parse_ptr_reply’:
c-ares/ares_parse_ptr_reply.c:134:11: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=]
           strncpy(aliases[aliascnt], rr_data, strlen(rr_data)+1);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
c-ares/ares_parse_ptr_reply.c:134:47: note: length computed here
           strncpy(aliases[aliascnt], rr_data, strlen(rr_data)+1);
                                               ^~~~~~~~~~~~~~~

This patch fixes this issue...

Copy link
Contributor

@gjasny gjasny left a comment

Shouldn't the type be something like size_t instead of int?

@Iniesta8 Iniesta8 force-pushed the Iniesta8:master branch from bec3220 to e7acbf3 Jun 12, 2018
@Iniesta8
Copy link
Contributor Author

@Iniesta8 Iniesta8 commented Jun 12, 2018

Yes, of course. Fixed.

@coveralls
Copy link

@coveralls coveralls commented Jun 12, 2018

Coverage Status

Coverage increased (+0.002%) to 92.953% when pulling e7acbf3 on Iniesta8:master into b93997b on c-ares:master.

@bradh352 bradh352 merged commit 7ebedab into c-ares:master Jun 23, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 92.953%
Details
@tavianator
Copy link

@tavianator tavianator commented Jul 18, 2018

Is this really a fix? rr_data_len still depends on the length of rr_data which makes it a strange argument for strncpy(). The current code might as well just be strcpy(aliases[aliascnt], rr_data).

@bradh352
Copy link
Member

@bradh352 bradh352 commented Jul 18, 2018

The original code was fine, there was no bug at all. The PR simply works around a GCC false-positive warning, so silencing a compiler false-positive.

@tavianator
Copy link

@tavianator tavianator commented Jul 18, 2018

Yeah looking at the original code it's obviously right. But I think either

strcpy(aliases[aliascnt], rr_data);

or

memcpy(aliases[aliascnt], rr_data, rr_data_len);

would be better replacements. Mainly because the warning is less likely to come back in the future if GCC starts seeing that rr_data_len has the same value it was complaining about before. Actually the allocation and copy could probably both be replaced by

aliases[aliascnt] = ares_strdup(rr_data);
@Iniesta8
Copy link
Contributor Author

@Iniesta8 Iniesta8 commented Jul 18, 2018

@tavianator: I think neither strcpy() nor strncpy() nor memcpy() is better or worse than the others. All these are just for fixing the false-positive warning.

However, your suggestion with ares_strdup() might be an alternative.

@RagnarPaulson
Copy link

@RagnarPaulson RagnarPaulson commented Jul 18, 2018

Seeing as strcpy/strncpy/memcpy all do a copy and ares_strdup() returns a pointer, I don't see that as a viable solution without changing the definition and use of 'aliases'.

@tavianator
Copy link

@tavianator tavianator commented Jul 18, 2018

@RagnarPaulson What? This works for me:

diff --git a/ares_parse_ptr_reply.c b/ares_parse_ptr_reply.c
index 29e22cb..54f3a6e 100644
--- a/ares_parse_ptr_reply.c
+++ b/ares_parse_ptr_reply.c
@@ -52,7 +52,6 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen, const void *addr,
   int aliascnt = 0;
   int alias_alloc = 8;
   char ** aliases;
-  size_t rr_data_len;
 
   /* Set *host to NULL for all failure cases. */
   *host = NULL;
@@ -125,15 +124,13 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen, const void *addr,
           if (hostname)
             ares_free(hostname);
           hostname = rr_data;
-          rr_data_len = strlen(rr_data)+1;
-          aliases[aliascnt] = ares_malloc(rr_data_len * sizeof(char));
+          aliases[aliascnt] = ares_strdup(rr_data);
           if (!aliases[aliascnt])
             {
               ares_free(rr_name);
               status = ARES_ENOMEM;
               break;
             }
-          strncpy(aliases[aliascnt], rr_data, rr_data_len);
           aliascnt++;
           if (aliascnt >= alias_alloc) {
             char **ptr;
@RagnarPaulson
Copy link

@RagnarPaulson RagnarPaulson commented Jul 18, 2018

Yeah, I'm a doofus. I didn't read your comment close enough when you said 'allocation and copy' . Sorry.

DronRathore added a commit to DronRathore/c-ares that referenced this pull request Mar 11, 2020
When using a modern GCC to compile c-ares, there is a stringop-overflow warning. 
This patch simply silences the false-positive warning, there is no actual code flaw.

Bug: c-ares#201
Fixed By: Andi Schnebinger @Iniesta8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.