Bug 7474 - unconditional use of st_blocks and st_blksize
Summary: unconditional use of st_blocks and st_blksize
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: Build environment (show other bugs)
Version: 3.5.6
Hardware: Other Other
: P3 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-28 02:43 UTC by Joachim Schmitz (mail address dead)
Modified: 2011-06-17 07:52 UTC (History)
1 user (show)

See Also:


Attachments
Patch (4.49 KB, patch)
2010-05-28 04:51 UTC, Joachim Schmitz (mail address dead)
no flags Details
untested alternative patch (692 bytes, patch)
2010-06-07 10:04 UTC, Björn Jacke
no flags Details
new patch for 3.5 (1.05 KB, patch)
2010-06-09 08:48 UTC, Björn Jacke
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joachim Schmitz (mail address dead) 2010-05-28 02:43:19 UTC
unconditional use of st_blocks and st_blksize in .../source3/client/client.c, ./source3/libsmb/clifile.c, ./source3/modules/vfs_streams_xattr.c and .../source3/lib/system.c.

Patch:

diff -u ./source3/client/client.c.orig ./source3/client/client.c
--- ./source3/client/client.c.orig	2010-05-17 06:51:23.000000000 -0500
+++ ./source3/client/client.c	2010-05-26 08:41:43.000000000 -0500
@@ -3185,10 +3185,16 @@
 
 	/* Print out the stat values. */
 	d_printf("File: %s\n", src);
+#ifdef HAVE_STAT_ST_BLOCKS
 	d_printf("Size: %-12.0f\tBlocks: %u\t%s\n",
 		(double)sbuf.st_ex_size,
 		(unsigned int)sbuf.st_ex_blocks,
 		filetype_to_str(sbuf.st_ex_mode));
+#else
+	d_printf("Size: %-12.0f\t%s\n",
+		(double)sbuf.st_ex_size,
+		filetype_to_str(sbuf.st_ex_mode));
+#endif 
 
 #if defined(S_ISCHR) && defined(S_ISBLK)
 	if (S_ISCHR(sbuf.st_ex_mode) || S_ISBLK(sbuf.st_ex_mode)) {
./source3/client/mount.cifs.c missing
diff -u ./source3/libsmb/clifile.c.orig ./source3/libsmb/clifile.c
--- ./source3/libsmb/clifile.c.orig	2010-05-17 06:51:23.000000000 -0500
+++ ./source3/libsmb/clifile.c	2010-05-26 08:56:24.000000000 -0500
@@ -867,12 +867,14 @@
 	}
 
 	sbuf->st_ex_size = IVAL2_TO_SMB_BIG_UINT(state->data,0);     /* total size, in bytes */
+#if defined (HAVE_STAT_ST_BLOCKS)
 	sbuf->st_ex_blocks = IVAL2_TO_SMB_BIG_UINT(state->data,8);   /* number of blocks allocated */
-#if defined (HAVE_STAT_ST_BLOCKS) && defined(STAT_ST_BLOCKSIZE)
+# if defined defined(STAT_ST_BLOCKSIZE)
 	sbuf->st_ex_blocks /= STAT_ST_BLOCKSIZE;
-#else
+# else
 	/* assume 512 byte blocks */
 	sbuf->st_ex_blocks /= 512;
+# endif
 #endif
 	sbuf->st_ex_ctime = interpret_long_date((char *)(state->data + 16));    /* time of last change */
 	sbuf->st_ex_atime = interpret_long_date((char *)(state->data + 24));    /* time of last access */
diff -u ./source3/modules/vfs_streams_xattr.c.orig ./source3/modules/vfs_streams_xattr.c
--- ./source3/modules/vfs_streams_xattr.c.orig	2010-05-17 06:51:23.000000000 -0500
+++ ./source3/modules/vfs_streams_xattr.c	2010-05-26 06:46:57.000000000 -0500
@@ -237,7 +237,9 @@
 	sbuf->st_ex_ino = stream_inode(sbuf, io->xattr_name);
 	sbuf->st_ex_mode &= ~S_IFMT;
         sbuf->st_ex_mode |= S_IFREG;
+#ifdef HAVE_STAT_ST_BLOCKS
         sbuf->st_ex_blocks = sbuf->st_ex_size % STAT_ST_BLOCKSIZE + 1;
+#endif
 
 	return 0;
 }
@@ -289,8 +291,10 @@
 	smb_fname->st.st_ex_ino = stream_inode(&smb_fname->st, xattr_name);
 	smb_fname->st.st_ex_mode &= ~S_IFMT;
         smb_fname->st.st_ex_mode |= S_IFREG;
+#ifdef HAVE_STAT_ST_BLOCKS
         smb_fname->st.st_ex_blocks =
 	    smb_fname->st.st_ex_size % STAT_ST_BLOCKSIZE + 1;
+#endif
 
 	result = 0;
  fail:
@@ -340,8 +344,10 @@
 	smb_fname->st.st_ex_ino = stream_inode(&smb_fname->st, xattr_name);
 	smb_fname->st.st_ex_mode &= ~S_IFMT;
         smb_fname->st.st_ex_mode |= S_IFREG;
+#ifdef HAVE_STAT_ST_BLOCKS
         smb_fname->st.st_ex_blocks =
 	    smb_fname->st.st_ex_size % STAT_ST_BLOCKSIZE + 1;
+#endif
 
 	result = 0;
 
diff -u ./source3/lib/system.c.orig ./source3/lib/system.c
--- ./source3/lib/system.c.orig	2010-05-17 06:51:23.000000000 -0500
+++ ./source3/lib/system.c	2010-05-27 07:13:48.000000000 -0500
@@ -534,8 +534,12 @@
 	dst->st_ex_mtime = get_mtimespec(src);
 	dst->st_ex_ctime = get_ctimespec(src);
 	make_create_timespec(src, dst, fake_dir_create_times);
+#ifdef HAVE_STAT_ST_BLKSIZE
 	dst->st_ex_blksize = src->st_blksize;
+#endif
+#ifdef HAVE_STAT_ST_BLOCKS
 	dst->st_ex_blocks = src->st_blocks;
+#endif
 
 #ifdef HAVE_STAT_ST_FLAGS
 	dst->st_ex_flags = src->st_flags;



Similar to the very old and long closed bug #550

Bye, Jojo
Comment 1 Björn Jacke 2010-05-28 03:28:11 UTC
for future patches - can you please attach them to the bug report instead of pasting inline?
Comment 2 Joachim Schmitz (mail address dead) 2010-05-28 03:50:39 UTC
(In reply to comment #1)
> for future patches - can you please attach them to the bug report instead of
> pasting inline?

I found some more spots where st_blocks and st_blksize is used. I will created attach a patch later.

bye, Jojo
Comment 3 Joachim Schmitz (mail address dead) 2010-05-28 04:50:00 UTC
here we go, properly #ifdef'ing them in ...source3/includes/includes.h reveals it's unconditional use in .../source3/smbd/smb2_create.c

Patch (maybe not the most clever one):

diff -u ./source3/include/includes.h.orig ./source3/include/includes.h
--- ./source3/include/includes.h.orig   2010-05-17 06:51:23.000000000 -0500
+++ ./source3/include/includes.h        2010-05-28 03:46:26.000000000 -0500
@@ -442,8 +459,12 @@
        struct timespec st_ex_btime; /* birthtime */
        /* Is birthtime real, or was it calculated ? */
        bool            st_ex_calculated_birthtime;
+#ifdef HAVE_ST_BLKSIZE
        blksize_t       st_ex_blksize;
+#endif
+#ifdef HAVE_ST_BLOCKS
        blkcnt_t        st_ex_blocks;
+#endif

        uint32_t        st_ex_flags;
        uint32_t        st_ex_mask;

diff -u ./source3/smbd/smb2_create.c.orig ./source3/smbd/smb2_create.c
--- ./source3/smbd/smb2_create.c.orig   2010-05-17 06:51:23.000000000 -0500
+++ ./source3/smbd/smb2_create.c        2010-05-28 04:06:22.000000000 -0500
@@ -699,8 +699,16 @@
                        get_change_timespec(smbreq->conn, result,
                                        result->fsp_name));
        state->out_allocation_size =
+#ifdef HAVE_ST_BLKSIZE
                        result->fsp_name->st.st_ex_blksize *
+#else
+                       STAT_ST_BLOCKSIZE *
+#endif
+#ifdef HAVE_ST_BLOCKS
                        result->fsp_name->st.st_ex_blocks;
+#else
+                       result->fsp_name->st.st_ex_size % STAT_ST_BLOCKSIZE + 1;
+#endif
        state->out_end_of_file = result->fsp_name->st.st_ex_size;
        if (state->out_file_attributes == 0) {
                state->out_file_attributes = FILE_ATTRIBUTE_NORMAL;


Full patch will be attached shortly...

Bye, Jojo
Comment 4 Joachim Schmitz (mail address dead) 2010-05-28 04:51:41 UTC
Created attachment 5745 [details]
Patch
Comment 5 Björn Jacke 2010-06-07 10:03:48 UTC
jojo, I think you mistyped HAVE_STAT_ST_BLKSIZE with HAVE_ST_BLKSIZE in the patch?

The intention of the st_ex struct is to eliminate as many #ifdef's as possible. Maybe something like the following patch also works for you?
Comment 6 Björn Jacke 2010-06-07 10:04:43 UTC
Created attachment 5761 [details]
untested alternative patch
Comment 7 Joachim Schmitz (mail address dead) 2010-06-07 11:26:40 UTC
(In reply to comment #5)
> jojo, I think you mistyped HAVE_STAT_ST_BLKSIZE with HAVE_ST_BLKSIZE in the
> patch?

Yes, I did, sorry


> The intention of the st_ex struct is to eliminate as many #ifdef's as possible.
> Maybe something like the following patch also works for you?

I'll give it a try, but it should work. Looks much leaner and I like it better then my version.

Bye, Jojo

Bye, Jojo
Comment 8 Joachim Schmitz (mail address dead) 2010-06-07 15:54:35 UTC
It compiles cleanly with your patch.

Bye, Joo
Comment 9 Björn Jacke 2010-06-07 16:19:11 UTC
thanks! It's fixed in master with 711a30aa61bb5f6a9b3970007bad8a70f411fb87.

Volker, can you please review that commit for cherry-pick to 3.5 and then reassign to Karorin?
Comment 10 Joachim Schmitz (mail address dead) 2010-06-08 09:12:56 UTC
Is it realy '%' to be used here rather than '/'? when calculating the number of blocks using size and blocksize I think it is:

dst->st_ex_blocks = src->st_size / STAT_ST_BLOCKSIZE + 1;

With the "+ 1" to cater for the remainder. I had stolen that from .../source3/modules/vfs_streams_xattr.c, but now believe it is wrong there too.

Also we should be using src->st_blksize, shouldn't we? 

dst->st_ex_blocks = src->st_size / src->st_blksize + 1;
Comment 11 Joachim Schmitz (mail address dead) 2010-06-08 09:19:54 UTC
Nonsense, I meant
dst->st_ex_blocks = src->st_size / dst->st_ex_blksize + 1;
Comment 12 Björn Jacke 2010-06-08 16:31:48 UTC
Good point, looks wrong to me too. That code has been inheritted since a while, initially from Darwin's samba stream module :-)

The "+ 1" should actually also just be applied if
"dst->st_ex_size % dst->st_ex_blksize > 0".
Comment 13 Joachim Schmitz (mail address dead) 2010-06-09 01:55:53 UTC
(In reply to comment #12)

> The "+ 1" should actually also just be applied if
> "dst->st_ex_size % dst->st_ex_blksize > 0".

True, but this is a corner case and only valid for systems that don't HAVE_STAT_ST_BLOCKS, so that possible little error wouldn't be to bad and that extra calculation not worth the efford, would it?
And better allocating a bit too much rather than too little.

in .../source3/modules/vfs_streams_xattr.c (in 3 places) we should get it right though and I have the feeling that .../source3/libsmb/clifile.c needs fixing too, as it totaly ignores the possible remainder when calculating the number of blocks allocated (line ~871). 

Bye, Jojo
Comment 14 Björn Jacke 2010-06-09 08:48:09 UTC
Created attachment 5775 [details]
new patch for 3.5

Volker, can you please review and reassign to Karolin?
Comment 15 Stefan Metzmacher 2010-08-16 07:07:44 UTC
Comment on attachment 5775 [details]
new patch for 3.5

I'm not sure if this is always correct:

dst->st_ex_blocks = src->st_size / dst->st_ex_blksize + 1;

I mean the '+1' should only be there if st_size isn't a multiple of st_ex_blksize.

metze
Comment 16 Stefan Metzmacher 2010-08-16 07:12:36 UTC
Comment on attachment 5775 [details]
new patch for 3.5

Ok, I read the other comments in the bug...
as it's just a corner case.
Comment 17 Karolin Seeger 2010-08-24 09:43:41 UTC
(In reply to comment #16)
> (From update of attachment 5775 [details])
> Ok, I read the other comments in the bug...
> as it's just a corner case.
> 

Metze, please clarify if I should pick the patch or not.
Shall I wait for Volker's review also?

Thanks!
Comment 18 Björn Jacke 2010-09-22 11:54:55 UTC
Metze's ack is sufficiant :-)
Comment 19 Karolin Seeger 2010-09-25 07:53:15 UTC
Pushed to v3-5-test.
Closing out bug report.

Thanks!
Comment 20 Joachim Schmitz (mail address dead) 2010-10-23 08:58:08 UTC
I think we still have a potential problem here, see patch:

diff -u ./source3/lib/system.c.orig ./source3/lib/system.c
--- ./source3/lib/system.c.orig 2010-10-07 11:41:16.000000000 -0500
+++ ./source3/lib/system.c      2010-10-12 12:15:39.000000000 -0500
@@ -537,7 +537,11 @@
 #ifdef HAVE_STAT_ST_BLKSIZE
        dst->st_ex_blksize = src->st_blksize;
 #else
+# ifdef STAT_ST_BLOCKSIZE
        dst->st_ex_blksize = STAT_ST_BLOCKSIZE;
+# else
+       dst->st_ex_blksize = 512; /* we have to assume something... */
+# endif
 #endif

 #ifdef HAVE_STAT_ST_BLOCKS

Bye, Jojo
Comment 21 Björn Jacke 2010-10-25 15:15:08 UTC
not required. STAT_ST_BLOCKSIZE has to be defined (see AC_DEFINE(...) in configure.in) - otherwise the code would not compile - which would require a fix in configure.in then.
Comment 22 Joachim Schmitz (mail address dead) 2010-10-25 23:15:42 UTC
Thanks for clarification