[PATCHES BUG 13446] smbd: Cache dfree information based on query path

[Jeremy Allison]

On Wed, May 23, 2018 at 01:37:52PM -0700, Christof Schmitt wrote:
> Here is the updated patch series including a test. The last patch should
> not be backported as it would change the VFS ABI.
> 
> Could you review this one?

Firstly, you've got some trailing whitespace (git-hook caught that for me).

In:

 uint64_t get_dfree_info(connection_struct *conn, struct smb_filename *fname,
                        uint64_t *bsize, uint64_t *dfree, uint64_t *dsize)
 {
        int dfree_cache_time = lp_dfree_cache_time(SNUM(conn));
-       struct dfree_cached_info *dfc = conn->dfree_info;
+       struct dfree_cached_info *dfc = NULL, dfc_new;
        uint64_t dfree_ret;
+       char tmpbuf[PATH_MAX], *full_path, *to_free, *key_path;
+       size_t len;
+       DATA_BLOB key, value;

....

Please split the new variables onto separate lines and initialize to NULL, e.g.

+       struct dfree_cached_info *dfc = NULL
+       struct dfree_cached_info dfc_new;
+       char tmpbuf[PATH_MAX];
+       char *full_path = NULL;
+       char *to_free = NULL;
+       char *key_path = NULL;

Ensure dfc_new is zeroed before use:

+       DBG_DEBUG("Creating dfree cache entry for %s\n", key_path);
+	ZERO_STRUCT(dfc_new); <-- add this.
+       dfc_new.bsize = *bsize;
+       dfc_new.dfree = *dfree;
+       dfc_new.dsize = *dsize;

This section:

+       if (VALID_STAT(fname->st) && S_ISREG(fname->st.st_ex_mode)) {
+               /*
+                * In case of a file use the parent directory to reduce number
+                * of cache entries.
+                */
+               bool ok;
+
+               ok = parent_dirname(talloc_tos(), full_path, &key_path, NULL);
+               if (!ok) {
+                       errno = ENOMEM;
+                       return -1;
+               }
+
+       } else {
+               key_path = full_path;
+       }

Made me think. In the 'if (!ok)' error case you need to TALLOC_FREE(to_free),
also, parent_dirname() will *always* allocate key_path, meaning on normal
exit on this case you need to TALOC_FREE(to_free), but you're not freeing key_path.

As you're done with 'to_free' if parent_dirname returns true, you might
want to change this to:

+       if (VALID_STAT(fname->st) && S_ISREG(fname->st.st_ex_mode)) {
+               /*
+                * In case of a file use the parent directory to reduce number
+                * of cache entries.
+                */
+               bool ok;
+
+               ok = parent_dirname(talloc_tos(), full_path, &key_path, NULL);
+               if (!ok) {
+			TALLOC_FREE(to_free);
+                       errno = ENOMEM;
+                       return -1;
+               }
+		TALLOC_FREE(to_free); /* We're done with full_path */
+		/* key_path is always a talloced object. */
+		to_free = key_path;
+
+       } else {
+		/* key_path might not be a talloced object. */
+               key_path = full_path;
+       }

That way you always exit with only *one* thing to free -> 'to_free'
and you note what paths talloc and what paths might not.

The reason I bring this up is in one path, key_path is a talloced
object, but in the other key_path could be pointing to the stack
allocated char tmpbuf[PATH_MAX] (probably will in fact).

I just worry in the future someone might add a TALLOC_FREE(key_path)
without reading or understanding the code carefully enough.

In the last patch where you move the struct dfree_cached_info *dfree_info,
add a comment to source3/include/vfs.h noting you've changed the VFS ABI,
e.g.

/* Version 39 - Remove SMB_VFS_FSYNC
                Only implement async versions. */
/* Version 39 - Remove SMB_VFS_READ
                All users are now pread or async versions. */
/* Version 39 - Remove SMB_VFS_WRITE
                All users are now pwrite or async versions. */
+ /* Version 39 - Remove struct dfree_cached_info pointer from
+		  connection struct */

#define SMB_VFS_INTERFACE_VERSION 39

Otherwise looks good ! Can you re-do those and resend to me,
and I'll push if happy.

Cheers,

	Jeremy.