Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/db: Fix copy into fixed size buffer issue in SQLite driver #4255

Merged
merged 9 commits into from
Sep 12, 2024

Conversation

ShubhamDesai
Copy link
Contributor

This pull request resolves a buffer overflow issue detected by Coverity Scan (CID 1501211).
strcpy is replaced with G_strlcpy

@github-actions github-actions bot added C Related code is in C database Related to database management module labels Aug 29, 2024
@ShubhamDesai ShubhamDesai changed the title drivers: copy into fixed size buffer issue sqlite: copy into fixed size buffer issue Aug 29, 2024
@ShubhamDesai ShubhamDesai changed the title sqlite: copy into fixed size buffer issue db: copy into fixed size buffer issue Aug 29, 2024
db/drivers/sqlite/db.c Outdated Show resolved Hide resolved
db/drivers/sqlite/db.c Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@wenzeslaus wenzeslaus changed the title db: copy into fixed size buffer issue lib/db: Fix copy into fixed size buffer issue in SQLite driver Sep 3, 2024
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned, in this case the G_strlcpy() and check nicely fits in one line, so no need to use a variable (len).

I'm not sure whether this should result in warning or error, the result is in this case truncated. @wenzeslaus or @neteler what do you say?

db/drivers/sqlite/db.c Outdated Show resolved Hide resolved
db/drivers/sqlite/db.c Outdated Show resolved Hide resolved
db/drivers/sqlite/db.c Outdated Show resolved Hide resolved
db/drivers/sqlite/db.c Outdated Show resolved Hide resolved
db/drivers/sqlite/db.c Outdated Show resolved Hide resolved
db/drivers/sqlite/db.c Outdated Show resolved Hide resolved
db/drivers/sqlite/db.c Outdated Show resolved Hide resolved
Shubham Vasudeo Desai and others added 3 commits September 9, 2024 14:18
@echoix
Copy link
Member

echoix commented Sep 10, 2024

As I mentioned, in this case the G_strlcpy() and check nicely fits in one line, so no need to use a variable (len).

I'm not sure whether this should result in warning or error, the result is in this case truncated. @wenzeslaus or @neteler what do you say?

I'm going to try a take on this.

Considering that this is library code, I would expect it to be more robust than a module or addon;
Considering also that it is for doing database (SQLite?), where database operations are expected to be fully completed as requested or not at all, (see ACID concepts);
Considering that using a warning when operations would end up truncating a database name, and continuing the execution to another database, not the intended one (for example a database file that matches the name of the truncated string on warning);
I think that if the preconditions aren't met, we should fail. We aren't supposed to be in situations where we are in buffer-overflow territory, but when we are, we might as well limit the damages and stopping soon.

Would that approach make sense? Is there a completely different point of view that I didn't consider?

@nilason
Copy link
Contributor

nilason commented Sep 10, 2024

We should probably issue warning and return DB_FAILED, but I need to take a closer look at this.

@nilason
Copy link
Contributor

nilason commented Sep 11, 2024

The database library has its own set of functions handling errors. A more proper way to handle these failures is illustrated by:

db_d_append_error("%s %s\n%s", _("Unable to open database:"), name3,
(char *)sqlite3_errmsg(sqlite));
db_d_report_error();
return DB_FAILED;

The modules are (relatively) short lived and if there is a failure it may exit the process with e.g. G_fatal_error. A caller to the library, on the other hand, may wish to do some cleaning up or make an alternative approach in case of failure.

@echoix echoix merged commit dda0fb2 into OSGeo:main Sep 12, 2024
26 checks passed
@neteler neteler added this to the 8.5.0 milestone Sep 12, 2024
Mahesh1998 pushed a commit to Mahesh1998/grass that referenced this pull request Sep 19, 2024
…#4255)

* drivers: copy into fixed size buffer issue

* Requested changes

* without variable

* Update db/drivers/sqlite/db.c

Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>

* Use Db statements

---------

Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C database Related to database management module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants