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

Warning fixes #227

Merged
merged 7 commits into from Mar 14, 2019

Conversation

Projects
None yet
2 participants
@rpavlik
Copy link
Member

rpavlik commented Mar 12, 2019

A number of small fixes to appease GCC 8.x on Debian Buster.

@rpavlik rpavlik requested a review from russell-taylor Mar 12, 2019

@russell-taylor
Copy link
Contributor

russell-taylor left a comment

Thanks for fixing these! I only saw one thing that needs to be changed, which was the dropping of the null character comment. Some other nits that could be addressed.

@@ -6564,7 +6562,7 @@ char *vrpn_copy_file_name(const char *filespecifier)
filename = NULL;
try {
filename = new char[len];
strncpy(filename, fp, len - 1);
strncpy(filename, fp, len);
filename[len - 1] = 0;

This comment has been minimized.

@russell-taylor

russell-taylor Mar 13, 2019

Contributor

Since the code uses strlen() to find the length and then adds 1 to it, I guess this will always include the trailing 0. In that case, doing the full-length copy makes sense, but then the following line setting the last character to 0 is no longer needed. This must have been copied from code that worked even with a non-null-terminated string.

This comment has been minimized.

@rpavlik

rpavlik Mar 13, 2019

Author Member

So this and the other ones were all fixes to warnings with clang-8. I hadn't seen them before so I might have goofed them up, but strncpy is notoriously prickly - see the article linked in the comment to vrpn_strcpy.

../vrpn_Connection.C: In function ‘char* vrpn_copy_file_name(const char*)’:
../vrpn_Connection.C:6567:14: warning: ‘char* strncpy(char*, const char*, size_t)’ output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
       strncpy(filename, fp, len - 1);
       ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
../vrpn_Connection.C:6563:21: note: length computed here
     len = 1 + strlen(fp);
               ~~~~~~^~~~

This comment has been minimized.

@russell-taylor

russell-taylor Mar 14, 2019

Contributor

It is true that this stops copying before the null character, but then the next line fills in the null character. The full-length copy is better, but also makes the following line superfluous.

// This is guaranteed to fit because of the new allocation above.
strncpy(d_mutexName, name, strlen(name));
strncpy(d_mutexName, name, n);

This comment has been minimized.

@russell-taylor

russell-taylor Mar 13, 2019

Contributor

This will fail to copy the terminating null character to the end of the string; it needs to be set back to n to make it copy that character.

This comment has been minimized.

@rpavlik

rpavlik Mar 13, 2019

Author Member

I thought n = 1 + strlen would mean that n includes the null.

This comment has been minimized.

@russell-taylor

russell-taylor Mar 14, 2019

Contributor

You're right. I was looking at the replaced line as if it were the new line. Doh!

@@ -509,12 +509,12 @@ extern bool vrpn_test_pack_unpack(void);

template <size_t charCount> void vrpn_strcpy(char (&to)[charCount], const char* pSrc)
{
// Copy the string — don’t copy too many bytes.
// Copy the string - don't copy too many bytes.
#ifdef _WIN32
strncpy_s(to, pSrc, charCount);

This comment has been minimized.

@russell-taylor

russell-taylor Mar 13, 2019

Contributor

Presumably the Window path should match the other path? Either n or n-1 seem like they would work in this case, with n-1 being more efficient.

This comment has been minimized.

@rpavlik

rpavlik Mar 13, 2019

Author Member

not really, see the article linked in the comment. strncpy is a minefield of inconsistency. This is the optimum version of this function I eventually came up with, perhaps we switch to it: https://github.com/rpavlik/util-headers/blob/master/util/FixedLengthStringFunctions.h

This comment has been minimized.

@russell-taylor

russell-taylor Mar 14, 2019

Contributor

The article doesn't say why we should not use the same length in strncpy_s as in strncpy, but it does point out that strncpy_s needs to be guarded in try..catch. This patch is an improvement, but I'll put it on my list to switch the windows path later.

int ret;
ret = d_ana->unregister_change_handler(this, handle_analog_update);
int ret = d_ana->unregister_change_handler(this, handle_analog_update);
(void)ret;

This comment has been minimized.

@russell-taylor

russell-taylor Mar 13, 2019

Contributor

I didn't know about this cool trick to use a variable without using it.

Maybe we just remove the variable altogether and ignore the return value from the function call?

This comment has been minimized.

@rpavlik

rpavlik Mar 13, 2019

Author Member

yeah, that's probably good.

The whole cast-to-void thing looks like so much of a hack, but it's been the way to do this for a long time I think. There's a newer "possibly_unused" attribute but I haven't seen any advantage to using it. Though, this could be wrapped in a macro:

#define VRPN_POSSIBLY_UNUSED(VAR) (void)(VAR)

VRPN_POSSIBLY_UNUSED(ret);
@rpavlik

This comment has been minimized.

Copy link
Member Author

rpavlik commented Mar 13, 2019

FWIW, here is approximately the warnings that were fixed - as I said, I might have fixed them wrong.
warnings.txt

@russell-taylor russell-taylor merged commit b6de71e into master Mar 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@russell-taylor russell-taylor deleted the warning-fixes branch Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.