[CDRIVER-2006] mongoc_gridfs_find_one_with_opts(): the error parameter is not initialized on failure Created: 29/Jan/17  Updated: 03/May/17  Resolved: 28/Mar/17

Status: Closed
Project: C Driver
Component/s: GridFS
Affects Version/s: 1.5.3
Fix Version/s: 1.7.0

Type: Bug Priority: Major - P3
Reporter: Arseny Vakhrushev Assignee: A. Jesse Jiryu Davis
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: File libmongoc4.c    
Issue Links:
Related
related to CDRIVER-895 Review GridFS errno behavior/document... Closed
is related to CDRIVER-2002 Given mongoc_gridfs_file_readv() retu... Closed

 Description   

And yet again we have a situation when despite a failed status, the error parameter is not initialized. Now starring mongoc_gridfs_find_one_with_opts().

I am maybe paranoid, but this lack of consistency is pretty serious. Adding to that, these types of errors can lead to anything from innocent garbage spills to breach in security to sporadic crashes, etc.

As a counter-measure, I'd suggest users explicitly initialize error containers with something like BSON_ERROR_INITIALIZER. It won't solve the inconsistency reporting problem, but at least it will prevent adverse consequences of uninitialized values.

The example:

#include <mongoc.h>
 
int main() {
	mongoc_client_t *client;
	mongoc_gridfs_t *gridfs;
	mongoc_gridfs_file_t *file;
 
	bson_t query = BSON_INITIALIZER;
	bson_error_t error;
 
	mongoc_init();
 
	client = mongoc_client_new("mongodb://127.0.0.1");
	BSON_ASSERT(client);
	gridfs = mongoc_client_get_gridfs(client, "test-gridfs", 0, &error);
	BSON_ASSERT(gridfs);
	BSON_ASSERT(mongoc_gridfs_drop(gridfs, &error)); // Make sure GridFS is clean
 
	file = mongoc_gridfs_find_one_with_opts(gridfs, &query, 0, &error);
	BSON_ASSERT(!file);
	
	printf("Domain: %u, code: %u, message: %s\n", error.domain, error.code, error.message);
 
	mongoc_gridfs_destroy(gridfs);
	mongoc_client_destroy(client);
	mongoc_cleanup();
	return 0;
}

The output:

$ ./a.out
Domain: 1740548756, code: 0, message: PO�gQ

Valgrind:

...
==759== Use of uninitialised value of size 8
==759==    at 0x53131EB: _itoa_word (in /usr/lib/libc-2.24.so)
==759==    by 0x5317909: vfprintf (in /usr/lib/libc-2.24.so)
==759==    by 0x531E278: printf (in /usr/lib/libc-2.24.so)
==759==    by 0x400B4A: main (libmongoc4.c:22)
==759==
==759== Conditional jump or move depends on uninitialised value(s)
==759==    at 0x53131F5: _itoa_word (in /usr/lib/libc-2.24.so)
==759==    by 0x5317909: vfprintf (in /usr/lib/libc-2.24.so)
==759==    by 0x531E278: printf (in /usr/lib/libc-2.24.so)
==759==    by 0x400B4A: main (libmongoc4.c:22)
==759==
==759== Conditional jump or move depends on uninitialised value(s)
==759==    at 0x5317A11: vfprintf (in /usr/lib/libc-2.24.so)
==759==    by 0x531E278: printf (in /usr/lib/libc-2.24.so)
==759==    by 0x400B4A: main (libmongoc4.c:22)
...



 Comments   
Comment by Githook User [ 28/Mar/17 ]

Author:

{u'username': u'ajdavis', u'name': u'A. Jesse Jiryu Davis', u'email': u'jesse@mongodb.com'}

Message: CDRIVER-2006 gridfs find_one clears error
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/465af7e5e7d44100e193f0b858c817fbc4ce7e7d

Comment by Arseny Vakhrushev [ 30/Jan/17 ]

I just thought... Is it really an error that a file is not found? It's much like a successful db.findOne() that returns no results, right? Or, if put another way, it's an empty mongoc_gridfs_file_list_t returned by mongoc_gridfs_find_with_opts(). In this case, it's definitely not an error because mongoc_gridfs_file_list_error() will return false for an empty list which is expected behaviour.

So, it is probably much better to just set error.domain = error.code = 0 in that case and explicitly mention in the manual that the user must check against error.code to determine if there's a real error.

I am pretty sure I saw a similar situation somewhere in mongoc (can't remember where exactly now) when a pointer to something is returned. And when it's NULL, it's not necessarily an error condition. You are probably aware better than me though.

Comment by Arseny Vakhrushev [ 29/Jan/17 ]

Right on, Jesse. It also might be a good idea to check all invocations of mongoc_cursor_error() throughout the code for similar concerns.

Comment by A. Jesse Jiryu Davis [ 29/Jan/17 ]

Thanks Arseny.

Here, we should check mongoc_cursor_error(list->cursor, &list->error). If that's false, we should set list->error to a new error code, MONGOC_ERROR_GRIDFS_FILE_NOT_FOUND.

https://github.com/mongodb/mongo-c-driver/blob/7f1dd3630d6bb4b8d5db4c2b865c328acd170012/src/mongoc/mongoc-gridfs-file-list.c#L99-L99

Generated at Wed Feb 07 21:13:52 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.