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

Add conan recipe #128

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Add conan recipe #128

merged 1 commit into from
Nov 22, 2023

Conversation

Myroendan
Copy link
Contributor

@Myroendan Myroendan force-pushed the add-conan-recipe branch 3 times, most recently from 6a3e73d to 7f5fe3d Compare November 20, 2023 16:08
Copy link
Owner

@utelle utelle left a comment

Choose a reason for hiding this comment

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

There are still 2 small issues:

  1. sqlite3mc_version.h has to be added to the list of public header files in CMakeList.txt
  2. For me it is unclear what exactly the purpose of the test is. Since the test creates a new database file (at least, when it is called for the first time), it will never fail.

CMakeLists.txt Outdated
@@ -352,7 +352,7 @@ if(SQLITE3MC_STATIC_RUNTIME_LINK)
endif()
message("Will build ${SQLITE3MC_TARGET} as ${SQLITE3MC_LINK}")

set_target_properties(${SQLITE3MC_TARGET} PROPERTIES PUBLIC_HEADER "${SQLITE3MC_PUBLIC_HEADERS}")
set_target_properties(${SQLITE3MC_TARGET} PROPERTIES PUBLIC_HEADER "${SQLITE3MC_PUBLIC_HEADERS};src/sqlite3mc_version.h")
Copy link
Owner

Choose a reason for hiding this comment

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

We will have to make sqlite3mc_version.h part of the list SQLITE3MC_PUBLIC_HEADERS, because it is included by sqlite3mc.h. Sorry, my fault - I simply missed this include file when I flagged the public header files in the list in issue #124.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sqlite3mc_version.h is currently not added to the SQLITE3MC_TARGET as a source file.

  • I can add it to SQLITE3MC_PUBLIC_HEADERS, but then it will be added as a source.
  • Or I can append it to SQLITE3MC_PUBLIC_HEADERS, but only after the sources were added to SQLITE3MC_TARGET.
    Which one should be?

Copy link
Owner

Choose a reason for hiding this comment

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

This sqlite3mc_version.h is currently not added to the SQLITE3MC_TARGET as a source file.

  • I can add it to SQLITE3MC_PUBLIC_HEADERS, but then it will be added as a source.

What's the problem with this approach? sqlite3mc_version.h is referenced from sqlite3mc.h and is therefore a public header file.

  • Or I can append it to SQLITE3MC_PUBLIC_HEADERS, but only after the sources were added to SQLITE3MC_TARGET.

Why?

  Which one should be?

IMHO it should be added to SQLITE3MC_PUBLIC_HEADERS.

Currently you have defined SQLITE3MC_PUBLIC_HEADERS as

set(SQLITE3MC_PUBLIC_HEADERS
  src/sqlite3.h
  src/sqlite3ext.h
  src/sqlite3mc.h
  src/sqlite3mc_vfs.h
  src/sqlite3userauth.h
)

because these are the header files a user of the library will need to be able to compile and build an application. sqlite3mc_version.h is included by sqlite3mc.h - that is, if it is missing compiling an application will fail. Therefore I think the right place to add it is SQLITE3MC_PUBLIC_HEADERS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright I'll add it to the public headers.
I was just wondering how did this compile so far. But I guess it's because the sqlite3mc_version.h was only used by sqlite3mc.h, and none of the other source files.

Copy link
Owner

Choose a reason for hiding this comment

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

Alright I'll add it to the public headers.

Thanks.

I was just wondering how did this compile so far. But I guess it's because the sqlite3mc_version.h was only used by sqlite3mc.h, and none of the other source files.

Well, sqlite3mc.c includes sqlite3mc.h which in turn includes sqlite3mc_version.h, and for example the version string (SQLITE3MC_VERSION_STRING) is used in function sqlite3mc_version() of sqlite3mc.c to return the version information. It compiles, because the header file resides in the same directory as the other header files and the source files.

return 1;
}

rc = sqlite3_key(db, key, strlen(key));
Copy link
Owner

Choose a reason for hiding this comment

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

sqlite3_key can only fail, if it is called for an in-memory or temporary database, or if the URI specifies an invalid cipher scheme. It does not fail if an invalid password was given. A wrong password will be detected only, after the database file was actually accessed, that is, after any SQL DML command was issued.

For a newly created database file it will almost never fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried the following:

  1. Creating a db
  2. Use sqlite3_key
  3. Creating a users table and insert one user
  4. Close the db
  5. Open the db again
  6. Use sqlite3_key with a wrong password

But it's returning with SQLITE_OK. Am I missing something?
If I try to access an already existing encrypted db with another exe, its working, I can't access it without the correct password. But I can't reproduce the same within one test program.

Copy link
Owner

Choose a reason for hiding this comment

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

I have tried the following:

  1. Creating a db
  2. Use sqlite3_key
  3. Creating a users table and insert one user
  4. Close the db
  5. Open the db again
  6. Use sqlite3_key with a wrong password

But it's returning with SQLITE_OK.

That is the normal behaviour. sqlite3_key will not return an error, even if the wrong password was provided. The reason is that only the encryption environment is set up, but the database file is not yet accessed. This is the default behaviour of SQLite. The database file is accessed only, if information is read or written from/to the database file.

Am I missing something?

Yes. To test whether the provided password is actually correct you need to perform a database query (like SELECT count(*) FROM SQLITE_SCHEMA;).

If I try to access an already existing encrypted db with another exe, its working, I can't access it without the correct password.

You mean sqlite3_key returns an error??? That can't be. An error is returned when you try to read or write data.

But I can't reproduce the same within one test program.

Just issue a query after calling sqlite3_key. This should result in an error if the wrong password was provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean sqlite3_key returns an error??? That can't be. An error is returned when you try to read or write data.

You're right, the query returned the error, not the sqlite3_key.

Just issue a query after calling sqlite3_key. This should result in an error if the wrong password was provided.

Thanks, it's working now. I'll update the PR this evening.

- Closes issue utelle#124: Conan package
- CMake: add sqlite3mc_version.h to public headers
@utelle
Copy link
Owner

utelle commented Nov 21, 2023

Thanks for addressing the open questions. I will review the changes tomorrow.

@Myroendan Myroendan requested a review from utelle November 22, 2023 06:36
@Myroendan
Copy link
Contributor Author

Thanks for addressing the open questions. I will review the changes tomorrow.

You're welcome. Thank you for your work.

Copy link
Owner

@utelle utelle left a comment

Choose a reason for hiding this comment

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

Most likely the test application could be further improved, but this can be done later on, if necessary. For now, I will approve the PR.

@utelle utelle merged commit 217cbdc into utelle:main Nov 22, 2023
9 checks passed
@utelle
Copy link
Owner

utelle commented Nov 22, 2023

A new sqlite3mc version will be published probably by the end of the week. Thereafter I will try to publish the conan package. In case I run into problems, I will get back to you, so that we can sort things out together.

@Myroendan Myroendan deleted the add-conan-recipe branch November 24, 2023 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants