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 Warnings in Release Build #458

Merged
merged 4 commits into from
Jun 2, 2023
Merged

Conversation

nosracd
Copy link
Contributor

@nosracd nosracd commented May 31, 2023

This PR:

  • Adds two addition CI jobs which run the Ubuntu builds with release flags (ea8a740)
  • Initializes variables with a nominal value (d9327cf)
  • Refactors size variables to an unsigned integer (b96cb3c)
  • Decreases the size of C strings so they can be appended to without warnings occurring (745cdd2)

Resolves #457.

This PR does not fix GCC 12 release build warnings (it is possible to use the Fedora target to produce those).

fixes warning "may be used uninitialized in this function [-Werror=maybe-uninitialized]"
Shouldn't ever be negative anyways.

Fixes the warning "argument 2 range [18446744071562067968, 18446744073709551615] exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]"
@nosracd nosracd requested a review from ihilt May 31, 2023 18:21
@nosracd nosracd self-assigned this May 31, 2023
@@ -747,7 +747,7 @@ static int emit_package(lcmgen_t *lcm, _package_contents_t *pc)
char **dirs = g_strsplit(pc->name, ".", 0);
char *pdname = build_filenamev(dirs);
char package_dir[PATH_MAX];
char package_dir_prefix[PATH_MAX];
char package_dir_prefix[PATH_MAX - 10]; // Must be less than package_dir to append to it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since we are setting the length of the destination buffer, we could use snprintf instead of reducing its length like this. snprintf is safer than sprintf in this case since it terminates the output with '\0', avoids the original error message, and allows us to check for truncation. (Although it does still check the length of the source buffer.) If truncation of the output occurs, snprintf would return PATH_MAX or greater in this case and if an error occurs it returns a negative value. I would assume in either of these cases we wouldn't be able to rely on the buffer to hold a valid path so logging an error and returning a negative value seems appropriates since under similar circumstances the same thing is done a little further down. If you agree with this approach, the same thing could be done in emit_python.c.

diff --git a/lcmgen/emit_lua.c b/lcmgen/emit_lua.c
index 080bc55..481a890 100644
--- a/lcmgen/emit_lua.c
+++ b/lcmgen/emit_lua.c
@@ -751,10 +751,20 @@ static int emit_package(lcmgen_t *lcm, _package_contents_t *pc)
     int have_package = dirs[0] != NULL;
     int write_init_lua = !getopt_get_bool(lcm->gopt, "lua-no-init");
 
-    sprintf(package_dir_prefix, "%s%s", getopt_get_string(lcm->gopt, "lpath"),
+    int ret = snprintf(package_dir_prefix, PATH_MAX, "%s%s", getopt_get_string(lcm->gopt, "lpath"),
             strlen(getopt_get_string(lcm->gopt, "lpath")) > 0 ? G_DIR_SEPARATOR_S : "");
-    sprintf(package_dir, "%s%s%s", package_dir_prefix, pdname,
+    if (ret >= PATH_MAX || ret < 0) {
+	    free(pdname);
+	    err("Could not create package directory prefix string\n");
+	    return -1;
+    }
+    ret = snprintf(package_dir, PATH_MAX, "%s%s%s", package_dir_prefix, pdname,
             have_package ? G_DIR_SEPARATOR_S : "");
+    if (ret >= PATH_MAX || ret < 0) {
+	    free(pdname);
+	    err("Could not create package directory string\n");
+	    return -1;
+    }
     free(pdname);
     if (strlen(package_dir)) {
         if (!g_file_test(package_dir, G_FILE_TEST_EXISTS)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like so? 68782e4

Fixes the warning "‘__builtin___sprintf_chk’ may write a terminating nul past the end of the destination [-Werror=format-overflow=]"
@ihilt ihilt merged commit dbe64e8 into lcm-proj:master Jun 2, 2023
@ihilt ihilt deleted the ci/release_build branch June 2, 2023 14:11
@tprk77 tprk77 mentioned this pull request Jun 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lcm 1.5.0 build issue on linux
2 participants