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 <ykaul@redhat.com>

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