Fix segfault when there's no config dir

Submitted by Paulo Zanoni on Nov. 25, 2011, 6:38 p.m.

Details

Message ID 1322246286-21777-1-git-send-email-przanoni@gmail.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Paulo Zanoni Nov. 25, 2011, 6:38 p.m.
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 hw/xfree86/parser/scan.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Sorry. I guess this should be pushed soon...

Patch hide | download patch | download mbox

diff --git a/hw/xfree86/parser/scan.c b/hw/xfree86/parser/scan.c
index 9b6c356..31c6499 100644
--- a/hw/xfree86/parser/scan.c
+++ b/hw/xfree86/parser/scan.c
@@ -855,8 +855,8 @@  OpenConfigDir(const char *path, const char *cmdline, const char *projroot,
 			free(dirpath);
 			dirpath = NULL;
 		}
-		while (num--)
-			free(list[num]);
+		while (num > 0)
+			free(list[--num]);
 		free(list);
 	}
 

Comments

Reviewed-by: Jeremy Huddleston <jeremyhu@apple.com>

On Nov 25, 2011, at 10:38, przanoni@gmail.com wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> hw/xfree86/parser/scan.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
> 
> Sorry. I guess this should be pushed soon...
> 
> diff --git a/hw/xfree86/parser/scan.c b/hw/xfree86/parser/scan.c
> index 9b6c356..31c6499 100644
> --- a/hw/xfree86/parser/scan.c
> +++ b/hw/xfree86/parser/scan.c
> @@ -855,8 +855,8 @@ OpenConfigDir(const char *path, const char *cmdline, const char *projroot,
> 			free(dirpath);
> 			dirpath = NULL;
> 		}
> -		while (num--)
> -			free(list[num]);
> +		while (num > 0)
> +			free(list[--num]);
> 		free(list);
> 	}
> 
> -- 
> 1.7.7.1
> 
> _______________________________________________
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>
On Fri, 25 Nov 2011 16:38:06 -0200, przanoni@gmail.com wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  hw/xfree86/parser/scan.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> Sorry. I guess this should be pushed soon...

I'd rather see the scandir return value checked for an error, otherwise
the code depends on the current implementation of AddConfigDirFiles
which seems rather fragile.
On Nov 25, 2011, at 3:44 PM, Keith Packard wrote:

> On Fri, 25 Nov 2011 16:38:06 -0200, przanoni@gmail.com wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> 
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>> hw/xfree86/parser/scan.c |    4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> Sorry. I guess this should be pushed soon...
> 
> I'd rather see the scandir return value checked for an error, otherwise
> the code depends on the current implementation of AddConfigDirFiles
> which seems rather fragile.

That seems like a second patch, independent of this one...
On Mon, 28 Nov 2011 09:09:42 -0800, Jeremy Huddleston <jeremyhu@apple.com> wrote:

> That seems like a second patch, independent of this one...

No, if the scandir return value was error-checked, then num would always
be >= 0 and the existing code would work fine.
2011/11/28 Keith Packard <keithp@keithp.com>:
> On Mon, 28 Nov 2011 09:09:42 -0800, Jeremy Huddleston <jeremyhu@apple.com> wrote:
>
>> That seems like a second patch, independent of this one...
>
> No, if the scandir return value was error-checked, then num would always
> be >= 0 and the existing code would work fine.

So, is this version ok? Do I need to do something else?  I received
reports on IRC and we also have bug #43286 about it.

Thank you,
Paulo
On Wed, 30 Nov 2011 14:16:33 -0200, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2011/11/28 Keith Packard <keithp@keithp.com>:
> > On Mon, 28 Nov 2011 09:09:42 -0800, Jeremy Huddleston <jeremyhu@apple.com> wrote:
> >
> >> That seems like a second patch, independent of this one...
> >
> > No, if the scandir return value was error-checked, then num would always
> > be >= 0 and the existing code would work fine.
> 
> So, is this version ok? Do I need to do something else?  I received
> reports on IRC and we also have bug #43286 about it.

I was thinking of something that explicitly handled the scandir error
return case in the obvious way:

@@ -852,6 +852,10 @@ OpenConfigDir(const char *path, const char *cmdline, const char *projroot,
 
 		/* match files named *.conf */
 		num = scandir(dirpath, &list, ConfigFilter, alphasort);
+		if (num < 0) {
+			num = 0;
+			list = NULL;
+		}
 		found = AddConfigDirFiles(dirpath, list, num);
 		if (!found) {
 			free(dirpath);
2011/12/1 Keith Packard <keithp@keithp.com>:
> @@ -852,6 +852,10 @@ OpenConfigDir(const char *path, const char *cmdline, const char *projroot,
>
>                /* match files named *.conf */
>                num = scandir(dirpath, &list, ConfigFilter, alphasort);
> +               if (num < 0) {
> +                       num = 0;
> +                       list = NULL;
> +               }
>                found = AddConfigDirFiles(dirpath, list, num);
>                if (!found) {
>                        free(dirpath);

Well, that solution would depend on AddConfigDirFiles behaving nicely
when called with list=NULL and num=0, which, from a certain point of
view, goes against your first comment on my initial patch.

Anyway, I'd be happy with any of the 3 solutions, since they all fix the bug.

>
> --
> keith.packard@intel.com
On Thu, 1 Dec 2011 13:40:57 -0200, Paulo Zanoni <przanoni@gmail.com> wrote:

> Anyway, I'd be happy with any of the 3 solutions, since they all fix
> the bug.

I've merged my version.
   fd976e4..3824f55  master -> master