[PATCH] git_odb_open ckeck for valid path to database

[PATCH] git_odb_open ckeck for valid path to database

[Esben Mose Hansen]

---
 src/errors.c                |    1 +
 src/git/common.h            |    3 +++
 src/odb.c                   |   13 +++++++++++++
 tests/t0204-opendb_errors.c |   38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 0 deletions(-)
 create mode 100644 tests/t0204-opendb_errors.c

diff --git a/src/errors.c b/src/errors.c
index f348997..074c01c 100644
--- a/src/errors.c
+++ b/src/errors.c
@@ -36,6 +36,7 @@ static struct {
 	{ GIT_ENOTOID, "Not a git oid" },
 	{ GIT_ENOTFOUND, "Object does not exist in the scope searched" },
 	{ GIT_ENOMEM, "Not enough space" },
+	{ GIT_ENOTDIR, "Not a directory" }
 };
 
 const char *git_strerror(int num)
diff --git a/src/git/common.h b/src/git/common.h
index c470e0e..96f2971 100644
--- a/src/git/common.h
+++ b/src/git/common.h
@@ -65,6 +65,9 @@
 /** Consult the OS error information. */
 #define GIT_EOSERR (GIT_ERROR - 4)
 
+/** The path is not a directory */
+#define GIT_ENOTDIR (GIT_ERROR - 5)
+
 GIT_BEGIN_DECL
 
 /** A revision traversal pool. */
diff --git a/src/odb.c b/src/odb.c
index 6d646a4..89d6d03 100644
--- a/src/odb.c
+++ b/src/odb.c
@@ -30,6 +30,10 @@
 #include "hash.h"
 #include "odb.h"
 
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
 #define GIT_PACK_NAME_MAX (5 + 40 + 1)
 struct git_pack {
 	git_odb *db;
@@ -1023,6 +1027,15 @@ int git_odb_open(git_odb **out, const char *objects_dir)
 		free(db);
 		return GIT_ERROR;
 	}
+	struct stat objects_dir_stat;
+	if (stat(db->objects_dir, &objects_dir_stat)) {
+		free(db);
+		return GIT_EOSERR;
+	}
+	if (!S_ISDIR(objects_dir_stat.st_mode)) {
+		free(db);
+		return GIT_ENOTDIR;
+	}
 
 	gitlck_init(&db->lock);
 
diff --git a/tests/t0204-opendb_errors.c b/tests/t0204-opendb_errors.c
new file mode 100644
index 0000000..e9b52c9
--- /dev/null
+++ b/tests/t0204-opendb_errors.c
@@ -0,0 +1,38 @@
+#include "test_lib.h"
+#include "test_helpers.h"
+#include <git/odb.h>
+#include "fileops.h"
+
+static char *odb_dir = "test-objects";
+
+static unsigned char one_bytes[] = {
+    0x31, 0x78, 0x9c, 0xe3, 0x02, 0x00, 0x00, 0x0b,
+    0x00, 0x0b,
+};
+
+static unsigned char one_data[] = {
+    0x0a,
+};
+
+static object_data one = {
+    one_bytes,
+    sizeof(one_bytes),
+    "8b137891791fe96927ad78e64b0aad7bded08bdc",
+    "blob",
+    "test-objects/8b",
+    "test-objects/8b/137891791fe96927ad78e64b0aad7bded08bdc",
+    one_data,
+    sizeof(one_data),
+};
+
+BEGIN_TEST(opendb_errors)
+    git_odb *db;
+    int error = git_odb_open(&db, odb_dir);
+    must_be_true(error == GIT_EOSERR);
+    must_be_true(errno == ENOENT);
+    must_pass(write_object_files(odb_dir, &one));
+    error = git_odb_open(&db, one.file);
+    must_be_true(error == GIT_ENOTDIR);
+
+    must_pass(remove_object_files(odb_dir, &one));
+END_TEST
-- 
1.6.3.3

Re: [PATCH] git_odb_open ckeck for valid path to database

[Ramsay Jones]

Esben Mose Hansen wrote:
> diff --git a/src/odb.c b/src/odb.c
> index 6d646a4..89d6d03 100644
> --- a/src/odb.c
> +++ b/src/odb.c
> @@ -30,6 +30,10 @@
>  #include "hash.h"
>  #include "odb.h"
>  
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +

None of these #includes should be necessary (they are not for me).
Also, "#include <unistd.h>" breaks on windows ;-)

>  #define GIT_PACK_NAME_MAX (5 + 40 + 1)
>  struct git_pack {
>  	git_odb *db;
> @@ -1023,6 +1027,15 @@ int git_odb_open(git_odb **out, const char *objects_dir)
>  		free(db);
>  		return GIT_ERROR;
>  	}
> +	struct stat objects_dir_stat;

This should be declared at the head of the function (or generally at the start of
a block/scope); ie we don't use this C99 facility. This breaks MSVC, which does
not understand C99.

> +	if (stat(db->objects_dir, &objects_dir_stat)) {
> +		free(db);
> +		return GIT_EOSERR;
> +	}
> +	if (!S_ISDIR(objects_dir_stat.st_mode)) {

This breaks on MSVC, since the MS headers do not define S_ISDIR. It's not difficult
to add; it's on my TODO list. (Andreas, I can add this later if you like)

However, I suspect it may be useful to add an gitfo_is_directory() function, or
something like it. Maybe. Andreas?

Thank you for the patch.

(I think Andreas would prefer to see libgit2 somewhere in the subject, perhaps
like [LIBGIT2 PATCH], otherwise he may miss you email)

ATB,
Ramsay Jones

Re: [LIBGIT2 PATCH] git_odb_open ckeck for valid path to database

[Esben Mose Hansen]

[-- Attachment #1: Type: Text/Plain, Size: 1245 bytes --]

On Monday 09 November 2009 20:16:15 Ramsay Jones wrote:

I have made 2 new patchsets: One 
0001-git_odb_open-check-for-valid-path-to-database.patch

which is simply the patch before with fixups, and another series

0001-Add-gitfo_is_diretory.patch
0002-git_odb_open-ckeck-for-valid-path-to-database-using-.patch

which adds a gitfo_is_directory (0001) and then uses this new function from 
git_odb_open (0002).

I hope I have done this git/email stuff correctly.

> This should be declared at the head of the function (or generally at the
>  start of a block/scope); ie we don't use this C99 facility. This breaks
>  MSVC, which does not understand C99.

C89 (?) is a tough monster, I see :)

> 
> > +	if (stat(db->objects_dir, &objects_dir_stat)) {
> > +		free(db);
> > +		return GIT_EOSERR;
> > +	}
> > +	if (!S_ISDIR(objects_dir_stat.st_mode)) {
> 
> This breaks on MSVC, since the MS headers do not define S_ISDIR. It's not
>  difficult to add; it's on my TODO list. (Andreas, I can add this later if
>  you like)

I have not fixed this. 


> (I think Andreas would prefer to see libgit2 somewhere in the subject,
>  perhaps like [LIBGIT2 PATCH], otherwise he may miss you email

OK.

Thanks for looking at my code.

-- 
Kind regards, Esben

[-- Attachment #2: 0001-git_odb_open-check-for-valid-path-to-database.patch --]
[-- Type: text/x-patch, Size: 3133 bytes --]

From bb29cbb2fc42ce46bd8a53ad02d291aa833846fa Mon Sep 17 00:00:00 2001
From: Esben Mose Hansen <kde@mosehansen.dk>
Date: Fri, 6 Nov 2009 10:32:16 +0100
Subject: [PATCH] git_odb_open check for valid path to database

---
 src/errors.c                |    1 +
 src/git/common.h            |    3 +++
 src/odb.c                   |    9 +++++++++
 tests/t0204-opendb_errors.c |   38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 0 deletions(-)
 create mode 100644 tests/t0204-opendb_errors.c

diff --git a/src/errors.c b/src/errors.c
index f348997..074c01c 100644
--- a/src/errors.c
+++ b/src/errors.c
@@ -36,6 +36,7 @@ static struct {
 	{ GIT_ENOTOID, "Not a git oid" },
 	{ GIT_ENOTFOUND, "Object does not exist in the scope searched" },
 	{ GIT_ENOMEM, "Not enough space" },
+	{ GIT_ENOTDIR, "Not a directory" }
 };
 
 const char *git_strerror(int num)
diff --git a/src/git/common.h b/src/git/common.h
index c470e0e..96f2971 100644
--- a/src/git/common.h
+++ b/src/git/common.h
@@ -65,6 +65,9 @@
 /** Consult the OS error information. */
 #define GIT_EOSERR (GIT_ERROR - 4)
 
+/** The path is not a directory */
+#define GIT_ENOTDIR (GIT_ERROR - 5)
+
 GIT_BEGIN_DECL
 
 /** A revision traversal pool. */
diff --git a/src/odb.c b/src/odb.c
index 6d646a4..a588c1b 100644
--- a/src/odb.c
+++ b/src/odb.c
@@ -1014,6 +1014,7 @@ int git_odb_exists(git_odb *db, const git_oid *id)
 
 int git_odb_open(git_odb **out, const char *objects_dir)
 {
+	struct stat objects_dir_stat;
 	git_odb *db = git__calloc(1, sizeof(*db));
 	if (!db)
 		return GIT_ERROR;
@@ -1023,6 +1024,14 @@ int git_odb_open(git_odb **out, const char *objects_dir)
 		free(db);
 		return GIT_ERROR;
 	}
+	if (stat(db->objects_dir, &objects_dir_stat)) {
+		free(db);
+		return GIT_EOSERR;
+	}
+	if (!S_ISDIR(objects_dir_stat.st_mode)) {
+		free(db);
+		return GIT_ENOTDIR;
+	}
 
 	gitlck_init(&db->lock);
 
diff --git a/tests/t0204-opendb_errors.c b/tests/t0204-opendb_errors.c
new file mode 100644
index 0000000..e9b52c9
--- /dev/null
+++ b/tests/t0204-opendb_errors.c
@@ -0,0 +1,38 @@
+#include "test_lib.h"
+#include "test_helpers.h"
+#include <git/odb.h>
+#include "fileops.h"
+
+static char *odb_dir = "test-objects";
+
+static unsigned char one_bytes[] = {
+    0x31, 0x78, 0x9c, 0xe3, 0x02, 0x00, 0x00, 0x0b,
+    0x00, 0x0b,
+};
+
+static unsigned char one_data[] = {
+    0x0a,
+};
+
+static object_data one = {
+    one_bytes,
+    sizeof(one_bytes),
+    "8b137891791fe96927ad78e64b0aad7bded08bdc",
+    "blob",
+    "test-objects/8b",
+    "test-objects/8b/137891791fe96927ad78e64b0aad7bded08bdc",
+    one_data,
+    sizeof(one_data),
+};
+
+BEGIN_TEST(opendb_errors)
+    git_odb *db;
+    int error = git_odb_open(&db, odb_dir);
+    must_be_true(error == GIT_EOSERR);
+    must_be_true(errno == ENOENT);
+    must_pass(write_object_files(odb_dir, &one));
+    error = git_odb_open(&db, one.file);
+    must_be_true(error == GIT_ENOTDIR);
+
+    must_pass(remove_object_files(odb_dir, &one));
+END_TEST
-- 
1.6.3.3


[-- Attachment #3: 0001-Add-gitfo_is_diretory.patch --]
[-- Type: text/x-patch, Size: 2026 bytes --]

From d1eb250c11a35e30aaaf0652a24daa27b6b90b8f Mon Sep 17 00:00:00 2001
From: Esben Mose Hansen <kde@mosehansen.dk>
Date: Tue, 10 Nov 2009 21:58:04 +0100
Subject: [PATCH 1/2] Add gitfo_is_diretory

---
 src/errors.c     |    1 +
 src/fileops.c    |   11 +++++++++++
 src/fileops.h    |    5 +++++
 src/git/common.h |    3 +++
 4 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/src/errors.c b/src/errors.c
index f348997..074c01c 100644
--- a/src/errors.c
+++ b/src/errors.c
@@ -36,6 +36,7 @@ static struct {
 	{ GIT_ENOTOID, "Not a git oid" },
 	{ GIT_ENOTFOUND, "Object does not exist in the scope searched" },
 	{ GIT_ENOMEM, "Not enough space" },
+	{ GIT_ENOTDIR, "Not a directory" }
 };
 
 const char *git_strerror(int num)
diff --git a/src/fileops.c b/src/fileops.c
index 5de89cb..47a8681 100644
--- a/src/fileops.c
+++ b/src/fileops.c
@@ -274,3 +274,14 @@ int gitfo_dirent(
 	closedir(dir);
 	return GIT_SUCCESS;
 }
+
+int gitfo_is_directory(const char* path) {
+	struct stat path_stat;
+	if (stat(path, &path_stat)) {
+		return GIT_EOSERR;
+	}
+	if (!S_ISDIR(path_stat.st_mode)) {
+		return GIT_ENOTDIR;
+	}
+	return GIT_SUCCESS;
+}
diff --git a/src/fileops.h b/src/fileops.h
index 02e4e5b..73eb72c 100644
--- a/src/fileops.h
+++ b/src/fileops.h
@@ -126,4 +126,9 @@ extern int gitfo_write_cached(gitfo_cache *ioc, void *buf, size_t len);
 extern int gitfo_flush_cached(gitfo_cache *ioc);
 extern int gitfo_close_cached(gitfo_cache *ioc);
 
+/**
+ * Check if path is a directory
+ */
+extern int gitfo_is_directory(const char* path);
+
 #endif /* INCLUDE_fileops_h__ */
diff --git a/src/git/common.h b/src/git/common.h
index c470e0e..96f2971 100644
--- a/src/git/common.h
+++ b/src/git/common.h
@@ -65,6 +65,9 @@
 /** Consult the OS error information. */
 #define GIT_EOSERR (GIT_ERROR - 4)
 
+/** The path is not a directory */
+#define GIT_ENOTDIR (GIT_ERROR - 5)
+
 GIT_BEGIN_DECL
 
 /** A revision traversal pool. */
-- 
1.6.3.3


[-- Attachment #4: 0002-git_odb_open-ckeck-for-valid-path-to-database-using-.patch --]
[-- Type: text/x-patch, Size: 936 bytes --]

From 4db79e006b722291d645f05a2340cd7d26a3d777 Mon Sep 17 00:00:00 2001
From: Esben Mose Hansen <kde@mosehansen.dk>
Date: Tue, 10 Nov 2009 21:58:36 +0100
Subject: [PATCH 2/2] git_odb_open ckeck for valid path to database using gitfo_is_directory

---
 src/odb.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/src/odb.c b/src/odb.c
index 6d646a4..709fe58 100644
--- a/src/odb.c
+++ b/src/odb.c
@@ -1014,6 +1014,7 @@ int git_odb_exists(git_odb *db, const git_oid *id)
 
 int git_odb_open(git_odb **out, const char *objects_dir)
 {
+	int status;
 	git_odb *db = git__calloc(1, sizeof(*db));
 	if (!db)
 		return GIT_ERROR;
@@ -1023,6 +1024,10 @@ int git_odb_open(git_odb **out, const char *objects_dir)
 		free(db);
 		return GIT_ERROR;
 	}
+	if ((status = gitfo_is_directory(db->objects_dir))) {
+		free(db);
+		return status;
+	}
 
 	gitlck_init(&db->lock);
 
-- 
1.6.3.3

Re: [LIBGIT2 PATCH] git_odb_open ckeck for valid path to database

[Esben Mose Hansen]

On Tuesday 10 November 2009 22:07:04 Esben Mose Hansen wrote:
> On Monday 09 November 2009 20:16:15 Ramsay Jones wrote:
> 
> I have made 2 new patchsets: One

Did these get lost in transit? Or am I going about it in the wrong way? :)

Thank you for your time

-- 
Kind regards, Esben

Re: [LIBGIT2 PATCH] git_odb_open ckeck for valid path to database

[Ramsay Jones]

Esben Mose Hansen wrote:
> On Tuesday 10 November 2009 22:07:04 Esben Mose Hansen wrote:
>> On Monday 09 November 2009 20:16:15 Ramsay Jones wrote:
>>
>> I have made 2 new patchsets: One
> 
> Did these get lost in transit? Or am I going about it in the wrong way? :)

Oh, sorry, I didn't notice that this was sent to me! *ahem*
(I thought/expected them to be addressed to Andreas)

Maybe you could resend the first patch, inline rather than as an attachment,
addressed to Andreas (ae@op5.com) so that he can comment on them (or commit
them).

ATB,
Ramsay Jones