From 0b128a55af782885c13b6f2531a72bddf7d5c84a Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Thu, 9 Feb 2012 16:23:15 +0000 Subject: [PATCH 01/13] Added a test case for PHP-310 --- tests/bug00310.phpt | 67 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 tests/bug00310.phpt diff --git a/tests/bug00310.phpt b/tests/bug00310.phpt new file mode 100644 index 000000000..5aed889a8 --- /dev/null +++ b/tests/bug00310.phpt @@ -0,0 +1,67 @@ +--TEST-- +Test for bug PHP-310: GridFS transaction issues +--FILE-- +selectDB($db_name); + + $GridFS = $mdb->getGridFS(); + + $GridFS->ensureIndex(array('filename' => 1), array("unique" => true)); + + $temporary_file_name = '/tmp/GridFS_test.txt'; + $temporary_file_data = '1234567890'; + file_put_contents($temporary_file_name, $temporary_file_data); + + echo "######################################\n"; + echo "# Saving files to GridFS\n"; + echo "######################################\n"; + for ($i = 0; $i < 3; $i++) { + try { + $new_saved_file_object_id = $GridFS->storeFile($temporary_file_name, array( '_id' => "file{$i}"), array('options' => 'safe')); + } + catch (MongoCursorException $e) { + echo "error message: ".$e->getMessage()."\n"; + echo "\nerror code: ".$e->getCode()."\n"; + } + + echo "[Saved file] New file id:".$new_saved_file_object_id."\n"; + } + + echo "\n"; + echo "######################################\n"; + echo "# Current documents in fs.files\n"; + echo "######################################\n"; + $cursor = $GridFS->findOne('/tmp/GridFS_test.txt'); + foreach($cursor as $this_cursor){ + echo "[file] [_id:".$this_cursor['_id']."] [filename:".$this_cursor['filename']."] [length:".$this_cursor['length']."] [chunkSize:".$this_cursor['chunkSize']."]\n"; + } + echo "\n"; + echo "######################################\n"; + echo "# Current documents in fs.chunks\n"; + echo "######################################\n"; + $cursor = $GridFS->chunks->find(); + foreach($cursor as $this_cursor){ + echo "[chunk] [_id:".$this_cursor['_id']."] [n:".$this_cursor['n']."] [files_id:".$this_cursor['files_id']."]\n"; + } +?> +--EXPECTF-- +###################################### +# Saving files to GridFS +###################################### +[Saved file] New file id:file0 +[Saved file] New file id:file1 +[Saved file] New file id:file2 + +###################################### +# Current documents in fs.files +###################################### +[file] [_id:file0] [filename:/tmp/GridFS_test.txt] [length:10] [chunkSize:262144] + +###################################### +# Current documents in fs.chunks +###################################### +[chunk] [_id:%s] [n:0] [files_id:file0] From bd1b24e0d4398617635e148811c423d7e413788e Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Thu, 9 Feb 2012 17:39:38 +0000 Subject: [PATCH 02/13] Fixed some more memory leaks with GridFS. --- gridfs.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/gridfs.c b/gridfs.c index b34c5d7b7..604f49acd 100644 --- a/gridfs.c +++ b/gridfs.c @@ -496,6 +496,7 @@ PHP_METHOD(MongoGridFS, storeFile) { zval *fh, *extra = 0, *options = 0; char *filename = 0; int chunk_num = 0, global_chunk_size = 0, size = 0, pos = 0, fd = -1, safe = 0; + int free_options = 0; FILE *fp = 0; zval temp; @@ -511,13 +512,6 @@ PHP_METHOD(MongoGridFS, storeFile) { return; } - if (!options) { - zval *opts; - MAKE_STD_ZVAL(opts); - array_init(opts); - options = opts; - } - if (Z_TYPE_P(fh) == IS_RESOURCE) { zend_rsrc_list_entry *le; php_stdio_stream_data *stdio_fptr = 0; @@ -577,6 +571,15 @@ PHP_METHOD(MongoGridFS, storeFile) { // chunkSize global_chunk_size = get_chunk_size(zfile TSRMLS_CC); + // options + if (!options) { + zval *opts; + MAKE_STD_ZVAL(opts); + array_init(opts); + options = opts; + free_options = 1; + } + // insert chunks while (pos < size || fp == 0) { int result = 0; @@ -607,13 +610,16 @@ PHP_METHOD(MongoGridFS, storeFile) { } } - if (safe && EG(exception)) { - return; - } + efree(buf); - chunk_num++; + if (safe && EG(exception)) { + if (free_options) { + zval_ptr_dtor(&options); + } + return; + } - efree(buf); + chunk_num++; if (fp == 0 && result < chunk_size) { break; @@ -627,6 +633,9 @@ PHP_METHOD(MongoGridFS, storeFile) { if (EG(exception)) { zval_ptr_dtor(&zfile); + if (free_options) { + zval_ptr_dtor(&options); + } return; } @@ -643,6 +652,10 @@ PHP_METHOD(MongoGridFS, storeFile) { zval_add_ref(&zid); zval_ptr_dtor(&zfile); + if (free_options) { + zval_ptr_dtor(&options); + } + RETURN_ZVAL(zid, 1, 1); } From a9420d891aaeaf3daedf2f7157b21658c31d2799 Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Thu, 16 Feb 2012 14:57:07 +0000 Subject: [PATCH 03/13] Allow GridFS files to be deleted by any type of ID. --- gridfs.c | 28 ++++++++++++++++------------ tests/gridfs-delete.phpt | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 12 deletions(-) create mode 100644 tests/gridfs-delete.phpt diff --git a/gridfs.c b/gridfs.c index 604f49acd..f49cfb6a0 100644 --- a/gridfs.c +++ b/gridfs.c @@ -839,21 +839,25 @@ PHP_METHOD(MongoGridFS, storeUpload) { * New GridFS API */ -PHP_METHOD(MongoGridFS, delete) { - zval *id, *criteria; +PHP_METHOD(MongoGridFS, delete) +{ + zval *id, *criteria; + char *str_id; + size_t str_len; + + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z", &id) == FAILURE) { + return; + } - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "O", &id, mongo_ce_Id) == FAILURE) { - return; - } + // Set up criteria array + MAKE_STD_ZVAL(criteria); + array_init(criteria); + add_assoc_zval(criteria, "_id", id); + zval_add_ref(&id); - MAKE_STD_ZVAL(criteria); - array_init(criteria); - add_assoc_zval(criteria, "_id", id); - zval_add_ref(&id); + MONGO_METHOD1(MongoGridFS, remove, return_value, getThis(), criteria); - MONGO_METHOD1(MongoGridFS, remove, return_value, getThis(), criteria); - - zval_ptr_dtor(&criteria); + zval_ptr_dtor(&criteria); } PHP_METHOD(MongoGridFS, get) { diff --git a/tests/gridfs-delete.phpt b/tests/gridfs-delete.phpt new file mode 100644 index 000000000..d39e35d42 --- /dev/null +++ b/tests/gridfs-delete.phpt @@ -0,0 +1,40 @@ +--TEST-- +GridFS: deleting files by ID +--FILE-- +selectDB($db_name); + $mdb->dropCollection("fs.files"); + $mdb->dropCollection("fs.chunks"); + + $GridFS = $mdb->getGridFS(); + + $temporary_file_name = '/tmp/GridFS_test.txt'; + $temporary_file_data = '1234567890'; + file_put_contents($temporary_file_name, $temporary_file_data); + + $ids = array( + "file0", + 452, + true, + new MongoID(), + array( 'a', 'b' => 5 ), + ); + + foreach ( $ids as $id ) + { + echo "Using ID:"; + var_dump( $id ); + $GridFS->storeFile($temporary_file_name, array( '_id' => $id)); + + echo "Items in DB: ", $GridFS->find()->count(), "\n"; + $GridFS->delete( $id ); + echo "Items in DB: ", $GridFS->find()->count(), "\n"; + + echo "\n"; + } +?> +--EXPECTF-- From d86885457f61b07accbf88a701c3d423926ec3f0 Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Thu, 16 Feb 2012 15:05:03 +0000 Subject: [PATCH 04/13] Fixed PHP-310: reverting with GridFS. --- gridfs.c | 114 ++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 92 insertions(+), 22 deletions(-) diff --git a/gridfs.c b/gridfs.c index f49cfb6a0..85e757ae6 100644 --- a/gridfs.c +++ b/gridfs.c @@ -31,6 +31,8 @@ #include "mongo_types.h" #include "db.h" +#include "ext/standard/php_smart_str.h" + typedef struct { FILE *file; int fd; /* underlying file descriptor */ @@ -454,7 +456,7 @@ static int setup_file_fields(zval *zfile, char *filename, int size TSRMLS_DC) { * - buf */ static int insert_chunk(zval *chunks, zval *zid, int chunk_num, char *buf, int chunk_size, zval *options TSRMLS_DC) { - zval temp; + zval temp; zval *zchunk, *zbin; // create chunk @@ -496,7 +498,7 @@ PHP_METHOD(MongoGridFS, storeFile) { zval *fh, *extra = 0, *options = 0; char *filename = 0; int chunk_num = 0, global_chunk_size = 0, size = 0, pos = 0, fd = -1, safe = 0; - int free_options = 0; + int free_options = 0, revert = 0; FILE *fp = 0; zval temp; @@ -591,32 +593,38 @@ PHP_METHOD(MongoGridFS, storeFile) { if (fp) { if ((int)fread(buf, 1, chunk_size, fp) < chunk_size) { zend_throw_exception_ex(mongo_ce_GridFSException, 0 TSRMLS_CC, "error reading file %s", filename); - return; + revert = 1; + efree(buf); + goto cleanup_on_failure; } pos += chunk_size; if (insert_chunk(chunks, zid, chunk_num, buf, chunk_size, options TSRMLS_CC) == FAILURE) { - break; + revert = 1; + efree(buf); + goto cleanup_on_failure; } } else { result = read(fd, buf, chunk_size); if (result == -1) { zend_throw_exception_ex(mongo_ce_GridFSException, 0 TSRMLS_CC, "error reading filehandle"); - return; + revert = 1; + efree(buf); + goto cleanup_on_failure; } pos += result; if (insert_chunk(chunks, zid, chunk_num, buf, result, options TSRMLS_CC) == FAILURE) { - break; + revert = 1; + efree(buf); + goto cleanup_on_failure; } } efree(buf); if (safe && EG(exception)) { - if (free_options) { - zval_ptr_dtor(&options); - } - return; + revert = 1; + goto cleanup_on_failure; } chunk_num++; @@ -632,11 +640,7 @@ PHP_METHOD(MongoGridFS, storeFile) { } if (EG(exception)) { - zval_ptr_dtor(&zfile); - if (free_options) { - zval_ptr_dtor(&options); - } - return; + goto cleanup_on_failure; } if (!fp) { @@ -645,18 +649,84 @@ PHP_METHOD(MongoGridFS, storeFile) { add_md5(zfile, zid, c TSRMLS_CC); - // insert file - MONGO_METHOD2(MongoCollection, insert, &temp, getThis(), zfile, options); + // insert file + if (!revert) { + zval *temp_return; - // cleanup - zval_add_ref(&zid); - zval_ptr_dtor(&zfile); + zval_add_ref(&options); + free_options = 1; + MAKE_STD_ZVAL(temp_return); + ZVAL_NULL(temp_return); + MONGO_METHOD2(MongoCollection, insert, temp_return, getThis(), zfile, options); + zval_ptr_dtor(&temp_return); + if (EG(exception)) { + revert = 1; + } + } + + if (!revert) { + RETVAL_ZVAL(zid, 1, 1); + } + +cleanup_on_failure: + // remove all inserted chunks and main file document + if (revert) { + zval *chunks, *files, *criteria_chunks, *criteria_files; + char *message = NULL; + smart_str tmp_message = { 0 }; + zval *temp_return; + + chunks = zend_read_property(mongo_ce_GridFS, getThis(), "chunks", strlen("chunks"), NOISY TSRMLS_CC); + if (EG(exception)) { + message = estrdup(Z_STRVAL_P(zend_read_property(mongo_ce_GridFSException, EG(exception), "message", strlen("message"), NOISY TSRMLS_CC))); + zend_clear_exception(TSRMLS_C); + } + + MAKE_STD_ZVAL(criteria_files); + array_init(criteria_files); + zval_add_ref(&zid); + add_assoc_zval(criteria_files, "_id", zid); + + MAKE_STD_ZVAL(criteria_chunks); + array_init(criteria_chunks); + zval_add_ref(&zid); + add_assoc_zval(criteria_chunks, "files_id", zid); + + MAKE_STD_ZVAL(temp_return); + ZVAL_NULL(temp_return); + MONGO_METHOD1(MongoCollection, remove, temp_return, chunks, criteria_chunks); + zval_ptr_dtor(&temp_return); + + MAKE_STD_ZVAL(temp_return); + ZVAL_NULL(temp_return); + MONGO_METHOD1(MongoCollection, remove, temp_return, getThis(), criteria_files); + zval_ptr_dtor(&temp_return); + + zval_ptr_dtor(&criteria_files); + zval_ptr_dtor(&criteria_chunks); + + // create the message for the exception + if (message) { + smart_str_appends(&tmp_message, "Could not store file: "); + smart_str_appends(&tmp_message, message); + smart_str_0(&tmp_message); + efree(message); + } else { + smart_str_appends(&tmp_message, "Could not store file for unknown reasons"); + smart_str_0(&tmp_message); + } + zend_throw_exception(mongo_ce_GridFSException, tmp_message.c, 0 TSRMLS_CC); + smart_str_free(&tmp_message); + RETVAL_FALSE; + } + + // cleanup + zval_add_ref(&zid); + zval_ptr_dtor(&zfile); if (free_options) { zval_ptr_dtor(&options); } - - RETURN_ZVAL(zid, 1, 1); } PHP_METHOD(MongoGridFS, findOne) { From 73b657a3e9ffeff3ec68bd2bc9afe732407cac1c Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Thu, 16 Feb 2012 16:51:08 +0000 Subject: [PATCH 05/13] Update test case. In order for this to work, you need safe mode. --- tests/bug00310.phpt | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/tests/bug00310.phpt b/tests/bug00310.phpt index 5aed889a8..05f51c270 100644 --- a/tests/bug00310.phpt +++ b/tests/bug00310.phpt @@ -7,6 +7,8 @@ Test for bug PHP-310: GridFS transaction issues $db_name = 'phpunit'; $m = new Mongo('mongodb://'.$ip.':'.$port.'/'.$db_name); $mdb = $m->selectDB($db_name); + $mdb->dropCollection("fs.files"); + $mdb->dropCollection("fs.chunks"); $GridFS = $mdb->getGridFS(); @@ -21,14 +23,14 @@ Test for bug PHP-310: GridFS transaction issues echo "######################################\n"; for ($i = 0; $i < 3; $i++) { try { - $new_saved_file_object_id = $GridFS->storeFile($temporary_file_name, array( '_id' => "file{$i}"), array('options' => 'safe')); + $new_saved_file_object_id = $GridFS->storeFile($temporary_file_name, array( '_id' => "file{$i}"), array('safe' => true)); + echo "[Saved file] New file id:".$new_saved_file_object_id."\n"; } - catch (MongoCursorException $e) { + catch (MongoException $e) { echo "error message: ".$e->getMessage()."\n"; - echo "\nerror code: ".$e->getCode()."\n"; + echo "error code: ".$e->getCode()."\n"; } - echo "[Saved file] New file id:".$new_saved_file_object_id."\n"; } echo "\n"; @@ -49,19 +51,3 @@ Test for bug PHP-310: GridFS transaction issues } ?> --EXPECTF-- -###################################### -# Saving files to GridFS -###################################### -[Saved file] New file id:file0 -[Saved file] New file id:file1 -[Saved file] New file id:file2 - -###################################### -# Current documents in fs.files -###################################### -[file] [_id:file0] [filename:/tmp/GridFS_test.txt] [length:10] [chunkSize:262144] - -###################################### -# Current documents in fs.chunks -###################################### -[chunk] [_id:%s] [n:0] [files_id:file0] From fa6d707d5593f1a5bf3bd3320946a6019bfb0e78 Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Sat, 18 Feb 2012 10:04:01 +0000 Subject: [PATCH 06/13] Force safe-mode for the meta-record of gridfs. --- gridfs.c | 3 +++ tests/bug00310.phpt | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/gridfs.c b/gridfs.c index 85e757ae6..41656cb00 100644 --- a/gridfs.c +++ b/gridfs.c @@ -582,6 +582,9 @@ PHP_METHOD(MongoGridFS, storeFile) { free_options = 1; } + // force safe mode + add_assoc_long(options, "safe", 1); + // insert chunks while (pos < size || fp == 0) { int result = 0; diff --git a/tests/bug00310.phpt b/tests/bug00310.phpt index 05f51c270..c8e438fa0 100644 --- a/tests/bug00310.phpt +++ b/tests/bug00310.phpt @@ -23,7 +23,7 @@ Test for bug PHP-310: GridFS transaction issues echo "######################################\n"; for ($i = 0; $i < 3; $i++) { try { - $new_saved_file_object_id = $GridFS->storeFile($temporary_file_name, array( '_id' => "file{$i}"), array('safe' => true)); + $new_saved_file_object_id = $GridFS->storeFile($temporary_file_name, array( '_id' => "file{$i}")); echo "[Saved file] New file id:".$new_saved_file_object_id."\n"; } catch (MongoException $e) { From 04e6a5c8e03d786f36ee9edc2110bc2b20dac0b1 Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Sat, 18 Feb 2012 10:04:56 +0000 Subject: [PATCH 07/13] Rename to correct bug number. --- tests/{bug00310.phpt => bug00320.phpt} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/{bug00310.phpt => bug00320.phpt} (97%) diff --git a/tests/bug00310.phpt b/tests/bug00320.phpt similarity index 97% rename from tests/bug00310.phpt rename to tests/bug00320.phpt index c8e438fa0..f74f58058 100644 --- a/tests/bug00310.phpt +++ b/tests/bug00320.phpt @@ -1,5 +1,5 @@ --TEST-- -Test for bug PHP-310: GridFS transaction issues +Test for bug PHP-320: GridFS transaction issues --FILE-- Date: Sat, 18 Feb 2012 10:08:34 +0000 Subject: [PATCH 08/13] Add expected result to test. --- gridfs.c | 4 ++-- tests/bug00320.phpt | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/gridfs.c b/gridfs.c index 41656cb00..2a6149b26 100644 --- a/gridfs.c +++ b/gridfs.c @@ -656,12 +656,12 @@ PHP_METHOD(MongoGridFS, storeFile) { if (!revert) { zval *temp_return; - zval_add_ref(&options); - free_options = 1; + Z_ADDREF_P(options); MAKE_STD_ZVAL(temp_return); ZVAL_NULL(temp_return); MONGO_METHOD2(MongoCollection, insert, temp_return, getThis(), zfile, options); zval_ptr_dtor(&temp_return); + Z_DELREF_P(options); if (EG(exception)) { revert = 1; } diff --git a/tests/bug00320.phpt b/tests/bug00320.phpt index f74f58058..c350417b7 100644 --- a/tests/bug00320.phpt +++ b/tests/bug00320.phpt @@ -51,3 +51,20 @@ Test for bug PHP-320: GridFS transaction issues } ?> --EXPECTF-- +###################################### +# Saving files to GridFS +###################################### +[Saved file] New file id:file0 +error message: Could not store file: E11000 duplicate key error index: phpunit.fs.files.$filename_1 dup key: { : "/tmp/GridFS_test.txt" } +error code: 0 +error message: Could not store file: E11000 duplicate key error index: phpunit.fs.files.$filename_1 dup key: { : "/tmp/GridFS_test.txt" } +error code: 0 +###################################### +# Current documents in fs.files +###################################### +[file] [_id:file0] [filename:/tmp/GridFS_test.txt] [length:10] [chunkSize:262144] + +###################################### +# Current documents in fs.chunks +###################################### +[chunk] [_id:%s] [n:0] [files_id:file0] From 5bbf4b00e6fb0584fbf63c728029a37d0b0f8a7f Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Sat, 18 Feb 2012 10:14:12 +0000 Subject: [PATCH 09/13] Fixed a memory leak when not freeing a cursor. --- gridfs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/gridfs.c b/gridfs.c index 2a6149b26..937ef72b0 100644 --- a/gridfs.c +++ b/gridfs.c @@ -482,6 +482,7 @@ static int insert_chunk(zval *chunks, zval *zid, int chunk_num, char *buf, int c else { MONGO_METHOD1(MongoCollection, insert, &temp, chunks, zchunk); } + zval_dtor(&temp); // increment counters zval_ptr_dtor(&zchunk); // zid->refcount = 1 From b31f07b413db3191919a3f26662c698fa427bf68 Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Sat, 18 Feb 2012 14:43:15 +0000 Subject: [PATCH 10/13] Also implement the cleanups for storeBytes --- gridfs.c | 150 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 90 insertions(+), 60 deletions(-) diff --git a/gridfs.c b/gridfs.c index 937ef72b0..b289e4615 100644 --- a/gridfs.c +++ b/gridfs.c @@ -337,6 +337,57 @@ static void add_md5(zval *zfile, zval *zid, mongo_collection *c TSRMLS_DC) { } } +static cleanup_broken_insert(INTERNAL_FUNCTION_PARAMETERS, zval *zid TSRMLS_CC) +{ + zval *chunks, *files, *criteria_chunks, *criteria_files; + char *message = NULL; + smart_str tmp_message = { 0 }; + zval *temp_return; + + chunks = zend_read_property(mongo_ce_GridFS, getThis(), "chunks", strlen("chunks"), NOISY TSRMLS_CC); + if (EG(exception)) { + message = estrdup(Z_STRVAL_P(zend_read_property(mongo_ce_GridFSException, EG(exception), "message", strlen("message"), NOISY TSRMLS_CC))); + zend_clear_exception(TSRMLS_C); + } + + MAKE_STD_ZVAL(criteria_files); + array_init(criteria_files); + zval_add_ref(&zid); + add_assoc_zval(criteria_files, "_id", zid); + + MAKE_STD_ZVAL(criteria_chunks); + array_init(criteria_chunks); + zval_add_ref(&zid); + add_assoc_zval(criteria_chunks, "files_id", zid); + + MAKE_STD_ZVAL(temp_return); + ZVAL_NULL(temp_return); + MONGO_METHOD1(MongoCollection, remove, temp_return, chunks, criteria_chunks); + zval_ptr_dtor(&temp_return); + + MAKE_STD_ZVAL(temp_return); + ZVAL_NULL(temp_return); + MONGO_METHOD1(MongoCollection, remove, temp_return, getThis(), criteria_files); + zval_ptr_dtor(&temp_return); + + zval_ptr_dtor(&criteria_files); + zval_ptr_dtor(&criteria_chunks); + + // create the message for the exception + if (message) { + smart_str_appends(&tmp_message, "Could not store file: "); + smart_str_appends(&tmp_message, message); + smart_str_0(&tmp_message); + efree(message); + } else { + smart_str_appends(&tmp_message, "Could not store file for unknown reasons"); + smart_str_0(&tmp_message); + } + zend_throw_exception(mongo_ce_GridFSException, tmp_message.c, 0 TSRMLS_CC); + smart_str_free(&tmp_message); + RETVAL_FALSE; +} + /* * Stores an array of bytes that may not have a filename, * such as data from a socket or stream. @@ -350,6 +401,7 @@ PHP_METHOD(MongoGridFS, storeBytes) { char *bytes = 0; int bytes_len = 0, chunk_num = 0, chunk_size = 0, global_chunk_size = 0, pos = 0; + int free_options = 0, revert = 0; zval temp; zval *extra = 0, *zid = 0, *zfile = 0, *chunks = 0, *options = 0; @@ -364,13 +416,6 @@ PHP_METHOD(MongoGridFS, storeBytes) { return; } - if (!options) { - zval *opts; - MAKE_STD_ZVAL(opts); - array_init(opts); - options = opts; - } - // file array object MAKE_STD_ZVAL(zfile); ZVAL_NULL(zfile); @@ -385,14 +430,30 @@ PHP_METHOD(MongoGridFS, storeBytes) { add_assoc_long(zfile, "length", bytes_len); } + // options + if (!options) { + zval *opts; + MAKE_STD_ZVAL(opts); + array_init(opts); + options = opts; + free_options = 1; + } + + // force safe mode + add_assoc_long(options, "safe", 1); + // insert chunks while (pos < bytes_len) { chunk_size = bytes_len-pos >= global_chunk_size ? global_chunk_size : bytes_len-pos; - insert_chunk(chunks, zid, chunk_num, bytes+pos, chunk_size, options TSRMLS_CC); - if (EG(exception)) { - return; - } + if (insert_chunk(chunks, zid, chunk_num, bytes+pos, chunk_size, options TSRMLS_CC) == FAILURE) { + revert = 1; + goto cleanup_on_failure; + } + if (EG(exception)) { + revert = 1; + goto cleanup_on_failure; + } // increment counters pos += chunk_size; @@ -404,11 +465,25 @@ PHP_METHOD(MongoGridFS, storeBytes) { // insert file MONGO_METHOD2(MongoCollection, insert, &temp, getThis(), zfile, options); + zval_dtor(&temp); + if (EG(exception)) { + revert = 1; + } - zval_add_ref(&zid); - zval_ptr_dtor(&zfile); +cleanup_on_failure: + if (!revert) { + RETVAL_ZVAL(zid, 1, 1); + } else { + cleanup_broken_insert(INTERNAL_FUNCTION_PARAM_PASSTHRU, zid TSRMLS_CC); + RETVAL_FALSE; + } - RETURN_ZVAL(zid, 1, 1); + zval_add_ref(&zid); + zval_ptr_dtor(&zfile); + + if (free_options) { + zval_ptr_dtor(&options); + } } /* add extra fields required for files: @@ -675,52 +750,7 @@ PHP_METHOD(MongoGridFS, storeFile) { cleanup_on_failure: // remove all inserted chunks and main file document if (revert) { - zval *chunks, *files, *criteria_chunks, *criteria_files; - char *message = NULL; - smart_str tmp_message = { 0 }; - zval *temp_return; - - chunks = zend_read_property(mongo_ce_GridFS, getThis(), "chunks", strlen("chunks"), NOISY TSRMLS_CC); - if (EG(exception)) { - message = estrdup(Z_STRVAL_P(zend_read_property(mongo_ce_GridFSException, EG(exception), "message", strlen("message"), NOISY TSRMLS_CC))); - zend_clear_exception(TSRMLS_C); - } - - MAKE_STD_ZVAL(criteria_files); - array_init(criteria_files); - zval_add_ref(&zid); - add_assoc_zval(criteria_files, "_id", zid); - - MAKE_STD_ZVAL(criteria_chunks); - array_init(criteria_chunks); - zval_add_ref(&zid); - add_assoc_zval(criteria_chunks, "files_id", zid); - - MAKE_STD_ZVAL(temp_return); - ZVAL_NULL(temp_return); - MONGO_METHOD1(MongoCollection, remove, temp_return, chunks, criteria_chunks); - zval_ptr_dtor(&temp_return); - - MAKE_STD_ZVAL(temp_return); - ZVAL_NULL(temp_return); - MONGO_METHOD1(MongoCollection, remove, temp_return, getThis(), criteria_files); - zval_ptr_dtor(&temp_return); - - zval_ptr_dtor(&criteria_files); - zval_ptr_dtor(&criteria_chunks); - - // create the message for the exception - if (message) { - smart_str_appends(&tmp_message, "Could not store file: "); - smart_str_appends(&tmp_message, message); - smart_str_0(&tmp_message); - efree(message); - } else { - smart_str_appends(&tmp_message, "Could not store file for unknown reasons"); - smart_str_0(&tmp_message); - } - zend_throw_exception(mongo_ce_GridFSException, tmp_message.c, 0 TSRMLS_CC); - smart_str_free(&tmp_message); + cleanup_broken_insert(INTERNAL_FUNCTION_PARAM_PASSTHRU, zid TSRMLS_DC); RETVAL_FALSE; } From 472026cd579fee0998b561963f032e1f4f809f3f Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Sat, 18 Feb 2012 14:44:10 +0000 Subject: [PATCH 11/13] Added a test case for storeBytes() case as well. --- tests/bug00320-2.phpt | 68 +++++++++++++++++++++++++++++++++++++++++++ tests/bug00320.phpt | 2 +- 2 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 tests/bug00320-2.phpt diff --git a/tests/bug00320-2.phpt b/tests/bug00320-2.phpt new file mode 100644 index 000000000..4d27b3895 --- /dev/null +++ b/tests/bug00320-2.phpt @@ -0,0 +1,68 @@ +--TEST-- +Test for bug PHP-320: GridFS transaction issues with storeBytes(). +--FILE-- +selectDB($db_name); + $mdb->dropCollection("fs.files"); + $mdb->dropCollection("fs.chunks"); + + $GridFS = $mdb->getGridFS(); + + $GridFS->ensureIndex(array('filename' => 1), array("unique" => true)); + + $temporary_file_data = '1234567890'; + + echo "######################################\n"; + echo "# Saving files to GridFS\n"; + echo "######################################\n"; + for ($i = 0; $i < 3; $i++) { + try { + $new_saved_file_object_id = $GridFS->storeBytes($temporary_file_data, array( '_id' => "file{$i}", 'filename' => '/tmp/GridFS_test.txt')); + echo "[Saved file] New file id:".$new_saved_file_object_id."\n"; + } + catch (MongoException $e) { + echo "error message: ".$e->getMessage()."\n"; + echo "error code: ".$e->getCode()."\n"; + } + + } + + echo "\n"; + echo "######################################\n"; + echo "# Current documents in fs.files\n"; + echo "######################################\n"; + $cursor = $GridFS->findOne('/tmp/GridFS_test.txt'); + foreach($cursor as $this_cursor){ + echo "[file] [_id:".$this_cursor['_id']."] [filename:".$this_cursor['filename']."] [length:".$this_cursor['length']."] [chunkSize:".$this_cursor['chunkSize']."]\n"; + } + echo "\n"; + echo "######################################\n"; + echo "# Current documents in fs.chunks\n"; + echo "######################################\n"; + $cursor = $GridFS->chunks->find(); + foreach($cursor as $this_cursor){ + echo "[chunk] [_id:".$this_cursor['_id']."] [n:".$this_cursor['n']."] [files_id:".$this_cursor['files_id']."]\n"; + } +?> +--EXPECTF-- +###################################### +# Saving files to GridFS +###################################### +[Saved file] New file id:file0 +error message: Could not store file: E11000 duplicate key error index: phpunit.fs.files.$filename_1 dup key: { : "/tmp/GridFS_test.txt" } +error code: 0 +error message: Could not store file: E11000 duplicate key error index: phpunit.fs.files.$filename_1 dup key: { : "/tmp/GridFS_test.txt" } +error code: 0 +###################################### +# Current documents in fs.files +###################################### +[file] [_id:file0] [filename:/tmp/GridFS_test.txt] [length:10] [chunkSize:262144] + +###################################### +# Current documents in fs.chunks +###################################### +[chunk] [_id:%s] [n:0] [files_id:file0] diff --git a/tests/bug00320.phpt b/tests/bug00320.phpt index c350417b7..1aeb148bf 100644 --- a/tests/bug00320.phpt +++ b/tests/bug00320.phpt @@ -1,5 +1,5 @@ --TEST-- -Test for bug PHP-320: GridFS transaction issues +Test for bug PHP-320: GridFS transaction issues with storeFile(). --FILE-- Date: Mon, 20 Feb 2012 10:16:27 +0000 Subject: [PATCH 12/13] Added a test case for GridFS::delete() with random types for the _id field. --- tests/gridfs-delete.phpt | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/gridfs-delete.phpt b/tests/gridfs-delete.phpt index d39e35d42..f2149412d 100644 --- a/tests/gridfs-delete.phpt +++ b/tests/gridfs-delete.phpt @@ -38,3 +38,30 @@ GridFS: deleting files by ID } ?> --EXPECTF-- +Using ID:string(5) "file0" +Items in DB: 1 +Items in DB: 0 + +Using ID:int(452) +Items in DB: 1 +Items in DB: 0 + +Using ID:bool(true) +Items in DB: 1 +Items in DB: 0 + +Using ID:object(MongoId)#5 (1) { + ["$id"]=> + string(24) "%s" +} +Items in DB: 1 +Items in DB: 0 + +Using ID:array(2) { + [0]=> + string(1) "a" + ["b"]=> + int(5) +} +Items in DB: 1 +Items in DB: 0 From bc6e7c5f0fcac7cb6f1320dfe76285e66dcbb711 Mon Sep 17 00:00:00 2001 From: Derick Rethans Date: Wed, 22 Feb 2012 12:09:51 +0000 Subject: [PATCH 13/13] Added splitting of the options array as we're modifying it, and make sure we only accept array zvals for options. --- gridfs.c | 12 ++++++------ tests/bug00320-2.phpt | 8 +++++++- tests/bug00320.phpt | 8 +++++++- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/gridfs.c b/gridfs.c index b289e4615..c419b73f1 100644 --- a/gridfs.c +++ b/gridfs.c @@ -412,9 +412,9 @@ PHP_METHOD(MongoGridFS, storeBytes) { chunks = zend_read_property(mongo_ce_GridFS, getThis(), "chunks", strlen("chunks"), NOISY TSRMLS_CC); ensure_gridfs_index(&temp, chunks TSRMLS_CC); - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|az", &bytes, &bytes_len, &extra, &options) == FAILURE) { - return; - } + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|aa/", &bytes, &bytes_len, &extra, &options) == FAILURE) { + return; + } // file array object MAKE_STD_ZVAL(zfile); @@ -586,9 +586,9 @@ PHP_METHOD(MongoGridFS, storeFile) { ensure_gridfs_index(&temp, chunks TSRMLS_CC); - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z|az", &fh, &extra, &options) == FAILURE) { - return; - } + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "z|aa/", &fh, &extra, &options) == FAILURE) { + return; + } if (Z_TYPE_P(fh) == IS_RESOURCE) { zend_rsrc_list_entry *le; diff --git a/tests/bug00320-2.phpt b/tests/bug00320-2.phpt index 4d27b3895..fdcb3f039 100644 --- a/tests/bug00320-2.phpt +++ b/tests/bug00320-2.phpt @@ -19,9 +19,10 @@ Test for bug PHP-320: GridFS transaction issues with storeBytes(). echo "######################################\n"; echo "# Saving files to GridFS\n"; echo "######################################\n"; + $options = array( 'safe' => false ); for ($i = 0; $i < 3; $i++) { try { - $new_saved_file_object_id = $GridFS->storeBytes($temporary_file_data, array( '_id' => "file{$i}", 'filename' => '/tmp/GridFS_test.txt')); + $new_saved_file_object_id = $GridFS->storeBytes($temporary_file_data, array( '_id' => "file{$i}", 'filename' => '/tmp/GridFS_test.txt'), $options); echo "[Saved file] New file id:".$new_saved_file_object_id."\n"; } catch (MongoException $e) { @@ -30,6 +31,7 @@ Test for bug PHP-320: GridFS transaction issues with storeBytes(). } } + var_dump( $options ); echo "\n"; echo "######################################\n"; @@ -57,6 +59,10 @@ error message: Could not store file: E11000 duplicate key error index: phpunit.f error code: 0 error message: Could not store file: E11000 duplicate key error index: phpunit.fs.files.$filename_1 dup key: { : "/tmp/GridFS_test.txt" } error code: 0 +array(1) { + ["safe"]=> + bool(false) +} ###################################### # Current documents in fs.files ###################################### diff --git a/tests/bug00320.phpt b/tests/bug00320.phpt index 1aeb148bf..956d0cb14 100644 --- a/tests/bug00320.phpt +++ b/tests/bug00320.phpt @@ -21,9 +21,10 @@ Test for bug PHP-320: GridFS transaction issues with storeFile(). echo "######################################\n"; echo "# Saving files to GridFS\n"; echo "######################################\n"; + $options = array( 'safe' => false ); for ($i = 0; $i < 3; $i++) { try { - $new_saved_file_object_id = $GridFS->storeFile($temporary_file_name, array( '_id' => "file{$i}")); + $new_saved_file_object_id = $GridFS->storeFile($temporary_file_name, array( '_id' => "file{$i}"), $options); echo "[Saved file] New file id:".$new_saved_file_object_id."\n"; } catch (MongoException $e) { @@ -32,6 +33,7 @@ Test for bug PHP-320: GridFS transaction issues with storeFile(). } } + var_dump( $options ); echo "\n"; echo "######################################\n"; @@ -59,6 +61,10 @@ error message: Could not store file: E11000 duplicate key error index: phpunit.f error code: 0 error message: Could not store file: E11000 duplicate key error index: phpunit.fs.files.$filename_1 dup key: { : "/tmp/GridFS_test.txt" } error code: 0 +array(1) { + ["safe"]=> + bool(false) +} ###################################### # Current documents in fs.files ######################################