[PATCH] cifs-utils: smbinfo.c: probably harmless wrong memset sizes + printf format correction

[PATCH] cifs-utils: smbinfo.c: probably harmless wrong memset sizes + printf format correction

[Adam Richter]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

The attached patch is my attempt at fixing two possibly harmless
complaints from "cppcheck --enable=warning" from the cifs-utils git
master branch version of smbinfo.c.

1. A printf format should have been "%u" instead of "%d" in print_quota.

2. An incorrect size was being passed to memset in thirteen nearly
identical places, each using "sizeof(qi)" when "sizeof(*qi)".  I am
not sure but I think these mistakes were probably harmless because the
memset calls might all be unnecessary and the sizes passed to each
memset call might never have been larger than it was supposed to be.

Because each of the effected memset calls was immediately preceded by
the malloc which allocated the data structure and because they each
ignored the possibility that malloc could fail, I made a function,
xmalloc0 to combine allocating the memory, zeroing it and exiting with
a non-zero exit value and a failure message on allocation failure
(which appears to be a fine way to handle the error in this program).
The function uses calloc, which could introduce an unnecessary
multiply, in the hopes that some calloc implementations may avoid the
memset in the case of freshly allocated memory from mmap, which would
probably be the case in this program, although I do not know if any
calloc implementations make this optimization.  Anyhow, at least this
way, the size of the data structure is only computed once in the
source code.

I realize that these memory allocations may all be for small data
structures that should be allocated on the stack and also may not need
to be cleared to all zeroes, but I did not want to delve into coding
style conventions for stack allocation in the CIFS source tree, and I
was not 100% certain that clearing the allocated memory was
unnecessary, although I do see other lines that explicitly initialize
some field in that that allocated memory to zero.  So, please feel
free to replace my changes with something better or one that involves
less code churn.

I should also warn that my only testing of these changes was to make
sure that "cppcheck --enable=warning" no longer complains, that the
file compiled without complaint (with cifs-utils standard "-Wall
-Wextra" arguments) and that "./smbinfo quote /dev/null" got past the
memory allocation to the (correct) ioctl error for /dev/null.

Also, I am not a CIFS developer and this may be the first time I have
submitted a patch, certainly the first time I remember, so please
forgive me and feel free to instruct me if I should be following some
different process to submit this patch.

Thanks in advance for considering this patch submission.

Adam

[-- Attachment #2: cifs-utils.smbinfo.memset_and_printf.diff --]
[-- Type: text/x-patch, Size: 4728 bytes --]

--- cifs-utils/smbinfo.c.orig	2019-05-25 14:19:56.758474588 -0700
+++ cifs-utils/smbinfo.c	2019-05-25 14:26:22.633256913 -0700
@@ -97,6 +97,18 @@ usage(char *name)
 	exit(1);
 }
 
+static void *
+xmalloc0(size_t nbytes)
+{
+	void *result = calloc(1, nbytes);
+	if (result == NULL) {
+		fprintf(stderr, "%s: failed to allocate %zu bytes.\n",
+			__func__, nbytes);
+		exit(1);
+	}
+	return result;
+}
+
 static void
 win_to_timeval(uint64_t smb2_time, struct timeval *tv)
 {
@@ -225,8 +237,7 @@ fsctlgetobjid(int f)
 
 	fstat(f, &st);
 
-	qi = malloc(sizeof(struct smb_query_info) + 64);
-	memset(qi, 0, sizeof(qi) + 64);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 64);
 	qi->info_type = 0x9009c;
 	qi->file_info_class = 0;
 	qi->additional_information = 0;
@@ -268,8 +279,7 @@ fileaccessinfo(int f)
 
 	fstat(f, &st);
 
-	qi = malloc(sizeof(struct smb_query_info) + 4);
-	memset(qi, 0, sizeof(qi) + 4);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 4);
 	qi->info_type = 0x01;
 	qi->file_info_class = 8;
 	qi->additional_information = 0;
@@ -322,8 +332,7 @@ filealigninfo(int f)
 {
 	struct smb_query_info *qi;
 
-	qi = malloc(sizeof(struct smb_query_info) + 4);
-	memset(qi, 0, sizeof(qi) + 4);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 4);
 	qi->info_type = 0x01;
 	qi->file_info_class = 17;
 	qi->additional_information = 0;
@@ -392,8 +401,7 @@ filebasicinfo(int f)
 {
 	struct smb_query_info *qi;
 
-	qi = malloc(sizeof(struct smb_query_info) + 40);
-	memset(qi, 0, sizeof(qi) + 40);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 40);
 	qi->info_type = 0x01;
 	qi->file_info_class = 4;
 	qi->additional_information = 0;
@@ -432,8 +440,7 @@ filestandardinfo(int f)
 {
 	struct smb_query_info *qi;
 
-	qi = malloc(sizeof(struct smb_query_info) + 24);
-	memset(qi, 0, sizeof(qi) + 24);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 24);
 	qi->info_type = 0x01;
 	qi->file_info_class = 5;
 	qi->additional_information = 0;
@@ -462,8 +469,7 @@ fileinternalinfo(int f)
 {
 	struct smb_query_info *qi;
 
-	qi = malloc(sizeof(struct smb_query_info) + 8);
-	memset(qi, 0, sizeof(qi) + 8);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 8);
 	qi->info_type = 0x01;
 	qi->file_info_class = 6;
 	qi->additional_information = 0;
@@ -505,8 +511,7 @@ filemodeinfo(int f)
 {
 	struct smb_query_info *qi;
 
-	qi = malloc(sizeof(struct smb_query_info) + 4);
-	memset(qi, 0, sizeof(qi) + 4);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 4);
 	qi->info_type = 0x01;
 	qi->file_info_class = 16;
 	qi->additional_information = 0;
@@ -535,8 +540,7 @@ filepositioninfo(int f)
 {
 	struct smb_query_info *qi;
 
-	qi = malloc(sizeof(struct smb_query_info) + 8);
-	memset(qi, 0, sizeof(qi) + 8);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 8);
 	qi->info_type = 0x01;
 	qi->file_info_class = 14;
 	qi->additional_information = 0;
@@ -565,8 +569,7 @@ fileeainfo(int f)
 {
 	struct smb_query_info *qi;
 
-	qi = malloc(sizeof(struct smb_query_info) + 4);
-	memset(qi, 0, sizeof(qi) + 4);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 4);
 	qi->info_type = 0x01;
 	qi->file_info_class = 7;
 	qi->additional_information = 0;
@@ -610,8 +613,7 @@ filefsfullsizeinfo(int f)
 {
 	struct smb_query_info *qi;
 
-	qi = malloc(sizeof(struct smb_query_info) + 32);
-	memset(qi, 0, sizeof(qi) + 32);
+	qi = xmalloc0(sizeof(struct smb_query_info) + 32);
 	qi->info_type = 0x02;
 	qi->file_info_class = 7;
 	qi->additional_information = 0;
@@ -634,8 +636,7 @@ fileallinfo(int f)
 
 	fstat(f, &st);
 
-	qi = malloc(sizeof(struct smb_query_info) + INPUT_BUFFER_LENGTH);
-	memset(qi, 0, sizeof(qi) + INPUT_BUFFER_LENGTH);
+	qi = xmalloc0(sizeof(struct smb_query_info) + INPUT_BUFFER_LENGTH);
 	qi->info_type = 0x01;
 	qi->file_info_class = 18;
 	qi->additional_information = 0;
@@ -862,8 +863,7 @@ secdesc(int f)
 
 	fstat(f, &st);
 
-	qi = malloc(sizeof(struct smb_query_info) + INPUT_BUFFER_LENGTH);
-	memset(qi, 0, sizeof(qi) + INPUT_BUFFER_LENGTH);
+	qi = xmalloc0(sizeof(struct smb_query_info) + INPUT_BUFFER_LENGTH);
 	qi->info_type = 0x03;
 	qi->file_info_class = 0;
 	qi->additional_information = 0x00000007; /* Owner, Group, Dacl */
@@ -892,7 +892,7 @@ one_more:
 
 	memcpy(&u32, &sd[off + 4], 4);
 	u32 = le32toh(u32);
-	printf("SID Length %d\n", u32);
+	printf("SID Length %u\n", u32);
 
 	memcpy(&u64, &sd[off + 8], 8);
 	win_to_timeval(le64toh(u64), &tv);
@@ -941,8 +941,7 @@ quota(int f)
 	char *buf;
 	int i;
 
-	qi = malloc(sizeof(struct smb_query_info) + INPUT_BUFFER_LENGTH);
-	memset(qi, 0, sizeof(struct smb_query_info) + INPUT_BUFFER_LENGTH);
+	qi = xmalloc0(sizeof(struct smb_query_info) + INPUT_BUFFER_LENGTH);
 	qi->info_type = 0x04;
 	qi->file_info_class = 0;
 	qi->additional_information = 0; /* Owner, Group, Dacl */

Re: [PATCH] cifs-utils: smbinfo.c: probably harmless wrong memset sizes + printf format correction

[Pavel Shilovsky]

Hi Adam,

Sorry it took me a while to look at this. The patch itself looks good
to me. Could you please add an appropriate description, create a patch
with "git format-patch" command and re-send it to the list? This would
allow me to merge it quickly. Submitting a PR on github against the
"next" branch is another good option.

--
Best regards,
Pavel Shilovsky

сб, 25 мая 2019 г. в 15:36, Adam Richter <adamrichter4@gmail.com>:
>
> The attached patch is my attempt at fixing two possibly harmless
> complaints from "cppcheck --enable=warning" from the cifs-utils git
> master branch version of smbinfo.c.
>
> 1. A printf format should have been "%u" instead of "%d" in print_quota.
>
> 2. An incorrect size was being passed to memset in thirteen nearly
> identical places, each using "sizeof(qi)" when "sizeof(*qi)".  I am
> not sure but I think these mistakes were probably harmless because the
> memset calls might all be unnecessary and the sizes passed to each
> memset call might never have been larger than it was supposed to be.
>
> Because each of the effected memset calls was immediately preceded by
> the malloc which allocated the data structure and because they each
> ignored the possibility that malloc could fail, I made a function,
> xmalloc0 to combine allocating the memory, zeroing it and exiting with
> a non-zero exit value and a failure message on allocation failure
> (which appears to be a fine way to handle the error in this program).
> The function uses calloc, which could introduce an unnecessary
> multiply, in the hopes that some calloc implementations may avoid the
> memset in the case of freshly allocated memory from mmap, which would
> probably be the case in this program, although I do not know if any
> calloc implementations make this optimization.  Anyhow, at least this
> way, the size of the data structure is only computed once in the
> source code.
>
> I realize that these memory allocations may all be for small data
> structures that should be allocated on the stack and also may not need
> to be cleared to all zeroes, but I did not want to delve into coding
> style conventions for stack allocation in the CIFS source tree, and I
> was not 100% certain that clearing the allocated memory was
> unnecessary, although I do see other lines that explicitly initialize
> some field in that that allocated memory to zero.  So, please feel
> free to replace my changes with something better or one that involves
> less code churn.
>
> I should also warn that my only testing of these changes was to make
> sure that "cppcheck --enable=warning" no longer complains, that the
> file compiled without complaint (with cifs-utils standard "-Wall
> -Wextra" arguments) and that "./smbinfo quote /dev/null" got past the
> memory allocation to the (correct) ioctl error for /dev/null.
>
> Also, I am not a CIFS developer and this may be the first time I have
> submitted a patch, certainly the first time I remember, so please
> forgive me and feel free to instruct me if I should be following some
> different process to submit this patch.
>
> Thanks in advance for considering this patch submission.
>
> Adam