Bug 10033 - call to mmap() not checked for failure in shared_mmap.c
call to mmap() not checked for failure in shared_mmap.c
Status: NEW
Product: Samba 4.0
Classification: Unclassified
Component: Other
4.0.7
All All
: P5 major
: ---
Assigned To: Andrew Bartlett
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-17 20:51 UTC by Bill Parker
Modified: 2013-07-26 19:58 UTC (History)
0 users

See Also:


Attachments
patch file for shared_mmap.c in /lib/replace/test (298 bytes, patch)
2013-07-17 20:51 UTC, Bill Parker
no flags Details
Revised patch for shared_mmap.c (1.34 KB, patch)
2013-07-26 19:54 UTC, Bill Parker
wp02855: review?
Details
Corrects file for README.Coding standard (1.81 KB, patch)
2013-07-26 19:56 UTC, Bill Parker
wp02855: review?
Details
Corrects file for README.Coding standard (648 bytes, patch)
2013-07-26 19:57 UTC, Bill Parker
wp02855: review?
Details
Corrects file for README.Coding standard (1.28 KB, patch)
2013-07-26 19:57 UTC, Bill Parker
wp02855: review?
Details
Corrects file for README.Coding standard (269 bytes, patch)
2013-07-26 19:58 UTC, Bill Parker
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Parker 2013-07-17 20:51:41 UTC
Created attachment 9058 [details]
patch file for shared_mmap.c in /lib/replace/test

Hello All,

	In reviewing source code files in Samba-4.0.7, I found
several instances where the library call 'mmap()' is not
checked for a return value of (int *)-1, indicating failure.

In directory 'samba-4.0.7/lib/replace/test', file 'shared_mmap.c',
I found an instance where mmap() was called without a test
for failure.  The patch file below adds the test:

--- shared_mmap.c.orig  2013-07-16 15:58:02.949981134 -0700
+++ shared_mmap.c       2013-07-16 15:58:30.811980273 -0700
@@ -39,6 +39,8 @@
                                   MAP_FILE | MAP_SHARED, 
                                   fd, 0);
 
+               if (buf == (int *)-1) exit(1);
+
                while (count-- && buf[9124] != 55732) sleep(1);
 
                if (count <= 0) exit(1);
				
I am attaching the patch file to this bug report

Bill Parker (wp02855 at gmail dot com)
Comment 1 Volker Lendecke 2013-07-23 07:08:04 UTC
Shouldn't the -1 be replaced by MAP_FAILED?
Comment 2 Christian Ambach 2013-07-23 09:41:12 UTC
Comment on attachment 9058 [details]
patch file for shared_mmap.c in /lib/replace/test

besides that, the patch does not comply to README.Coding.. it uses multiple statements on one line and no braces after the if statement
Comment 3 Bill Parker 2013-07-23 15:59:57 UTC
(In reply to comment #1)
> Shouldn't the -1 be replaced by MAP_FAILED?

That is a mistake on my part, but further down in the same
file, the -1 is used rather than MAP_FAILED (inconsistent).
Comment 4 Bill Parker 2013-07-23 16:02:25 UTC
(In reply to comment #2)
> Comment on attachment 9058 [details]
> patch file for shared_mmap.c in /lib/replace/test
> 
> besides that, the patch does not comply to README.Coding.. it uses multiple
> statements on one line and no braces after the if statement

Does this not also apply to the existing lines in shared_mmap.c below:

while (count-- && buf[9124] != 55732) sleep(1);

                if (count <= 0) exit(1);


I don't really see the difference between the if (count <=0) exit(1);
and             if (buf == (int *)-1) exit(1);

right?
Comment 5 Andrew Bartlett 2013-07-23 19:41:54 UTC
Much code in samba does not comply with the most modern README.Coding rules, but we try to make progress by improving the code as we change it.  A second patch to fix up other clear violations in would be much appreciated.
Comment 6 Bill Parker 2013-07-23 21:28:56 UTC
Do you want me to go through the entire file for which the previous mmap()
patch was submitted and fix it all?  I can do this, but it will take some
time.


On Tue, Jul 23, 2013 at 12:41 PM, <samba-bugs@samba.org> wrote:

> https://bugzilla.samba.org/show_bug.cgi?id=10033
>
> --- Comment #5 from Andrew Bartlett <abartlet@samba.org> 2013-07-23
> 19:41:54 UTC ---
> Much code in samba does not comply with the most modern README.Coding
> rules,
> but we try to make progress by improving the code as we change it.  A
> second
> patch to fix up other clear violations in would be much appreciated.
>
> --
> Configure bugmail: https://bugzilla.samba.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.
>
Comment 7 Bill Parker 2013-07-26 19:54:52 UTC
Created attachment 9077 [details]
Revised patch for shared_mmap.c

This patch file now corresponds to the README.Coding file in
Samba-4.0.7.
Comment 8 Bill Parker 2013-07-26 19:56:19 UTC
Created attachment 9078 [details]
Corrects file for README.Coding standard

This patch file converts incoherent_mmap.c so that it follows
coding standards in README.Coding
Comment 9 Bill Parker 2013-07-26 19:57:11 UTC
Created attachment 9079 [details]
Corrects file for README.Coding standard

This patch file converts os2_delete.c so that it follows
coding standards in README.Coding
Comment 10 Bill Parker 2013-07-26 19:57:59 UTC
Created attachment 9080 [details]
Corrects file for README.Coding standard

This patch file converts snprintf.c so that it follows
coding standards in README.Coding
Comment 11 Bill Parker 2013-07-26 19:58:33 UTC
Created attachment 9081 [details]
Corrects file for README.Coding standard

This patch file converts strptime.c so that it follows
coding standards in README.Coding