From dc6e6b71f87f6f89bb0b69816e92779595d716bd Mon Sep 17 00:00:00 2001 From: Yaniv Kaul Date: Tue, 21 Aug 2018 19:23:01 +0300 Subject: changelog xlator: strncpy()->sprintf(), reduce strlen()'s xlators/features/changelog/lib/src/gf-changelog-journal-handler.c xlators/features/changelog/lib/src/gf-changelog.c xlators/features/changelog/src/changelog-helpers.c xlators/features/changelog/src/changelog-misc.h strncpy may not be very efficient for short strings copied into a large buffer: If the length of src is less than n, strncpy() writes additional null bytes to dest to ensure that a total of n bytes are written. Instead, use snprintf(). Where possible, ensure there's no truncation of the output. Also: - save the result of strlen() and re-use it when possible. - move from strlen to SLEN (sizeof() ) for const strings. - switch a strncpy to a memcpy. Compile-tested only! Change-Id: Ia7a52bce0b243613ad910192ec163c93d944e077 updates: bz#1193929 Signed-off-by: Yaniv Kaul --- .../lib/src/gf-changelog-journal-handler.c | 36 ++++++++++-------- xlators/features/changelog/lib/src/gf-changelog.c | 5 ++- xlators/features/changelog/src/changelog-helpers.c | 43 ++++++++++++++-------- xlators/features/changelog/src/changelog-misc.h | 8 ++-- 4 files changed, 55 insertions(+), 37 deletions(-) diff --git a/xlators/features/changelog/lib/src/gf-changelog-journal-handler.c b/xlators/features/changelog/lib/src/gf-changelog-journal-handler.c index 9c1a498f655..bdb410030f6 100644 --- a/xlators/features/changelog/lib/src/gf-changelog-journal-handler.c +++ b/xlators/features/changelog/lib/src/gf-changelog-journal-handler.c @@ -456,7 +456,7 @@ gf_changelog_decode (xlator_t *this, gf_changelog_journal_t *jnl, size_t elen = 0; char buffer[1024] = {0,}; - CHANGELOG_GET_HEADER_INFO (from_fd, buffer, 1024, encoding, + CHANGELOG_GET_HEADER_INFO (from_fd, buffer, sizeof (buffer), encoding, major_version, minor_version, elen); if (encoding == -1) /* unknown encoding */ goto out; @@ -521,8 +521,9 @@ gf_changelog_publish (xlator_t *this, char to_path[PATH_MAX] = {0,}; struct stat stbuf = {0,}; - (void) snprintf (to_path, PATH_MAX, "%s%s", - jnl->jnl_current_dir, basename (from_path)); + if (snprintf (to_path, PATH_MAX, "%s%s", jnl->jnl_current_dir, + basename (from_path)) >= PATH_MAX) + return -1; /* handle zerob file that won't exist in current */ ret = sys_stat (to_path, &stbuf); @@ -532,8 +533,9 @@ gf_changelog_publish (xlator_t *this, goto out; } - (void) snprintf (dest, PATH_MAX, "%s%s", - jnl->jnl_processing_dir, basename (from_path)); + if (snprintf (dest, PATH_MAX, "%s%s", jnl->jnl_processing_dir, + basename (from_path)) >= PATH_MAX) + return -1; ret = sys_rename (to_path, dest); if (ret) { @@ -561,6 +563,13 @@ gf_changelog_consume (xlator_t *this, char dest[PATH_MAX] = {0,}; char to_path[PATH_MAX] = {0,}; + if (snprintf (to_path, PATH_MAX, "%s%s", jnl->jnl_current_dir, + basename (from_path)) >= PATH_MAX) + goto out; + if (snprintf (dest, PATH_MAX, "%s%s", jnl->jnl_processing_dir, + basename (from_path)) >= PATH_MAX) + goto out; + ret = sys_stat (from_path, &stbuf); if (ret || !S_ISREG(stbuf.st_mode)) { ret = -1; @@ -582,11 +591,6 @@ gf_changelog_consume (xlator_t *this, goto out; } - (void) snprintf (to_path, PATH_MAX, "%s%s", - jnl->jnl_current_dir, basename (from_path)); - (void) snprintf (dest, PATH_MAX, "%s%s", - jnl->jnl_processing_dir, basename (from_path)); - fd2 = open (to_path, O_CREAT | O_TRUNC | O_RDWR, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); if (fd2 < 0) { @@ -948,8 +952,9 @@ gf_changelog_init_history (xlator_t *this, goto dealloc_hist; } - (void) strncpy (jnl->hist_jnl->jnl_brickpath, brick_path, PATH_MAX-1); - jnl->hist_jnl->jnl_brickpath[PATH_MAX-1] = 0; + if (snprintf (jnl->hist_jnl->jnl_brickpath, PATH_MAX, "%s", + brick_path) >= PATH_MAX) + goto dealloc_hist; for (i = 0; i < 256; i++) { jnl->hist_jnl->rfc3986_space_newline[i] = @@ -999,6 +1004,10 @@ gf_changelog_journal_init (void *xl, struct gf_brick_spec *brick) if (!jnl) goto error_return; + if (snprintf (jnl->jnl_brickpath, PATH_MAX, "%s", + brick->brick_path) >= PATH_MAX) + goto dealloc_private; + if (sys_stat (scratch_dir, &buf) && errno == ENOENT) { ret = mkdir_p (scratch_dir, 0600, _gf_true); if (ret) @@ -1017,9 +1026,6 @@ gf_changelog_journal_init (void *xl, struct gf_brick_spec *brick) goto dealloc_private; } - (void) strncpy (jnl->jnl_brickpath, brick->brick_path, PATH_MAX-1); - jnl->jnl_brickpath[PATH_MAX-1] = 0; - /* RFC 3986 {de,en}coding */ for (i = 0; i < 256; i++) { jnl->rfc3986_space_newline[i] = diff --git a/xlators/features/changelog/lib/src/gf-changelog.c b/xlators/features/changelog/lib/src/gf-changelog.c index 6ad6c63740d..8198560e736 100644 --- a/xlators/features/changelog/lib/src/gf-changelog.c +++ b/xlators/features/changelog/lib/src/gf-changelog.c @@ -374,8 +374,9 @@ gf_setup_brick_connection (xlator_t *this, entry->connstate = GF_CHANGELOG_CONN_STATE_PENDING; entry->notify = brick->filter; - (void) strncpy (entry->brick, brick->brick_path, PATH_MAX-1); - entry->brick[PATH_MAX-1] = 0; + if (snprintf (entry->brick, PATH_MAX, "%s", brick->brick_path) + >= PATH_MAX) + goto free_entry; entry->this = this; entry->invokerxl = xl; diff --git a/xlators/features/changelog/src/changelog-helpers.c b/xlators/features/changelog/src/changelog-helpers.c index bfd2a29bdb1..28c72a8f9d4 100644 --- a/xlators/features/changelog/src/changelog-helpers.c +++ b/xlators/features/changelog/src/changelog-helpers.c @@ -290,10 +290,11 @@ htime_update (xlator_t *this, ret = -1; goto out; } - strncpy (changelog_path, buffer, PATH_MAX); - len = strlen (changelog_path); - changelog_path[len] = '\0'; /* redundant */ - + len = snprintf(changelog_path, PATH_MAX, "%s", buffer); + if (len >= PATH_MAX) { + ret = -1; + goto out; + } if (changelog_write (priv->htime_fd, (void*) changelog_path, len+1 ) < 0) { gf_msg (this->name, GF_LOG_ERROR, 0, CHANGELOG_MSG_HTIME_ERROR, @@ -302,11 +303,15 @@ htime_update (xlator_t *this, goto out; } - snprintf (x_value, sizeof x_value, "%lu:%d", - ts, priv->rollover_count); + len = snprintf (x_value, sizeof (x_value), "%lu:%d", + ts, priv->rollover_count); + if (len >= sizeof (x_value)) { + ret = -1; + goto out; + } if (sys_fsetxattr (priv->htime_fd, HTIME_KEY, x_value, - strlen (x_value), XATTR_REPLACE)) { + len, XATTR_REPLACE)) { gf_smsg (this->name, GF_LOG_ERROR, errno, CHANGELOG_MSG_HTIME_ERROR, "Htime xattr updation failed with XATTR_REPLACE", @@ -314,7 +319,7 @@ htime_update (xlator_t *this, NULL); if (sys_fsetxattr (priv->htime_fd, HTIME_KEY, x_value, - strlen (x_value), 0)) { + len, 0)) { gf_smsg (this->name, GF_LOG_ERROR, errno, CHANGELOG_MSG_HTIME_ERROR, "Htime xattr updation failed", @@ -366,7 +371,7 @@ cl_is_empty (xlator_t *this, int fd) goto out; } - CHANGELOG_GET_HEADER_INFO (fd, buffer, 1024, encoding, + CHANGELOG_GET_HEADER_INFO (fd, buffer, sizeof (buffer), encoding, major_version, minor_version, elen); if (elen == stbuf.st_size) { @@ -390,8 +395,8 @@ out: int update_path (xlator_t *this, char *cl_path) { - char low_cl[] = "changelog"; - char up_cl[] = "CHANGELOG"; + const char low_cl[] = "changelog"; + const char up_cl[] = "CHANGELOG"; char *found = NULL; int ret = -1; @@ -403,7 +408,7 @@ update_path (xlator_t *this, char *cl_path) "Could not find CHANGELOG in changelog path"); goto out; } else { - strncpy(found, low_cl, strlen(low_cl)); + memcpy(found, low_cl, sizeof (low_cl) - 1); } ret = 0; @@ -574,9 +579,11 @@ find_current_htime (int ht_dir_fd, const char *ht_dir_path, char *ht_file_bname) CHANGELOG_MSG_SCAN_DIR_FAILED, "scandir failed"); } else if (cnt > 0) { - strncpy (ht_file_bname, namelist[cnt - 1]->d_name, NAME_MAX); - ht_file_bname[NAME_MAX - 1] = 0; - + if (snprintf (ht_file_bname, NAME_MAX, "%s", + namelist[cnt - 1]->d_name) >= NAME_MAX) { + ret = -1; + goto out; + } if (sys_fsetxattr (ht_dir_fd, HTIME_CURRENT, ht_file_bname, strlen (ht_file_bname), 0)) { gf_msg (this->name, GF_LOG_ERROR, errno, @@ -2055,7 +2062,11 @@ resolve_pargfid_to_path (xlator_t *this, const uuid_t pgfid, ret = -1; goto out; } - strncpy (pre_dir_name, result, sizeof(pre_dir_name)); + if (snprintf (pre_dir_name, len + 1, "%s", result) + >= len + 1) { + ret = -1; + goto out; + } gf_uuid_parse (pgfidstr, tmp_gfid); gf_uuid_copy (pargfid, tmp_gfid); diff --git a/xlators/features/changelog/src/changelog-misc.h b/xlators/features/changelog/src/changelog-misc.h index 93af201879e..e96533f7365 100644 --- a/xlators/features/changelog/src/changelog-misc.h +++ b/xlators/features/changelog/src/changelog-misc.h @@ -86,13 +86,13 @@ } while (0) #define CHANGELOG_FILL_HTIME_DIR(changelog_dir, path) do { \ - strncpy (path, changelog_dir, sizeof (path) - 1); \ - strcat (path, "/htime"); \ + snprintf (path, sizeof (path), "%s/htime", \ + changelog_dir); \ } while(0) #define CHANGELOG_FILL_CSNAP_DIR(changelog_dir, path) do { \ - strncpy (path, changelog_dir, sizeof (path) - 1); \ - strcat (path, "/csnap"); \ + snprintf (path, sizeof (path), "%s/csnap", \ + changelog_dir); \ } while(0) /** * everything after 'CHANGELOG_TYPE_METADATA_XATTR' are internal types -- cgit