You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "cdaudt (via GitHub)" <gi...@apache.org> on 2023/03/31 17:03:18 UTC
[GitHub] [arrow-adbc] cdaudt opened a new pull request, #570: fix (c/driver-manager): Protect against uninitialized AdbcError
cdaudt opened a new pull request, #570:
URL: https://github.com/apache/arrow-adbc/pull/570
ADBC C API functions should initialize AdbcError struct passed into them instead of assuming that the caller did so. Given that these are "output-only" type parameters there is usually no expectation that they need to be zeroed coming into API calls.
This PR updates driver-manager. If all looks good and it gets merged, I'll follow up with equivalent PRs for the drivers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1564299060
I think for self-consistency we should stick with requiring that the user zero out all structs before use. This is maybe less convenient but makes intent clearer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] cdaudt commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "cdaudt (via GitHub)" <gi...@apache.org>.
cdaudt commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1492725121
> Ah, you're right. Then checking for null should be enough.
okay - I've resubmitted the PR with the null check + rename AdbcErrorInit -> AdbcErrorReset (and added a testcase for nullptr being passed into AdbcError). I did mess up the amend and ended up with an extra merge commit (haven't done a PR in a little while). Let me know if it looks okay or I should resubmit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] cdaudt commented on a diff in pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "cdaudt (via GitHub)" <gi...@apache.org>.
cdaudt commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1154952119
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -597,6 +628,7 @@ AdbcStatusCode AdbcStatementSetSqlQuery(struct AdbcStatement* statement,
AdbcStatusCode AdbcStatementSetSubstraitPlan(struct AdbcStatement* statement,
const uint8_t* plan, size_t length,
struct AdbcError* error) {
+ AdbcErrorInit(error);
Review Comment:
After seeing @kou comments below I'll rename the fn to AdbcErrorReset and check for null as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1660862613
In light of bugs like https://github.com/apache/arrow-adbc/issues/729, effectively the implementation requires that all structs must be zero-initialized (Golang-based drivers require that all inputs must be initialized). #946/#954 will only exacerbate this, so I think we will have to keep things as-is. Thank you for raising this behavior with us!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] cdaudt commented on a diff in pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "cdaudt (via GitHub)" <gi...@apache.org>.
cdaudt commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1154943175
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -597,6 +628,7 @@ AdbcStatusCode AdbcStatementSetSqlQuery(struct AdbcStatement* statement,
AdbcStatusCode AdbcStatementSetSubstraitPlan(struct AdbcStatement* statement,
const uint8_t* plan, size_t length,
struct AdbcError* error) {
+ AdbcErrorInit(error);
Review Comment:
It is small - I can replace with a direct call to memset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] github-actions[bot] commented on pull request #570: fix (c/driver-manager): Protect against uninitialized AdbcError
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1492272950
:warning: Please follow the [Conventional Commits format in CONTRIBUTING.md](https://github.com/apache/arrow-adbc/blob/main/CONTRIBUTING.md) for PR titles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1154985653
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -57,6 +57,11 @@ void GetWinError(std::string* buffer) {
#endif // defined(_WIN32)
+// Struct initializers
+static void AdbcErrorInit(struct AdbcError* error) {
+ std::memset(error, 0, sizeof(*error));
Review Comment:
The struggle is that we don't want to assume libc malloc/free (possibly we could/should have). You can see this pattern in the Arrow C Data Interface as well where all structs have an explicit release callback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] cdaudt commented on a diff in pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "cdaudt (via GitHub)" <gi...@apache.org>.
cdaudt commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1170838937
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -86,6 +86,47 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
}
+TEST_F(DriverManager, UninitializedError) {
+ struct AdbcDatabase database;
+ struct AdbcError invalid_err;
+ std::memset(&database, 0, sizeof(database));
+
+ // Force junk into AdbcError going into calls to
+ // simulate API calls using uninitialized AdbcError structs
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err),
+ IsStatus(ADBC_STATUS_INVALID_STATE, &invalid_err));
Review Comment:
Yes, there are a few missing releases (in my patch and other tests). Updating the patchset to add these.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1492609625
Also, CC @zeroshade if you have any opinions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1513944595
Probably because of the exact issue that Kou pointed out; it makes it easier to leak memory if you don't check the error (though of course, you _should_ be checking the error).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] kou commented on a diff in pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1206264112
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -83,9 +83,58 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
AdbcDatabaseSetOption(&database, "entrypoint", "ThisSymbolDoesNotExist", &error),
IsOkStatus(&error));
ASSERT_EQ(ADBC_STATUS_INTERNAL, AdbcDatabaseInit(&database, &error));
+ if (error.release) error.release(&error);
Review Comment:
Why do we need this `if`?
```suggestion
error.release(&error);
```
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -83,9 +83,58 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
AdbcDatabaseSetOption(&database, "entrypoint", "ThisSymbolDoesNotExist", &error),
IsOkStatus(&error));
ASSERT_EQ(ADBC_STATUS_INTERNAL, AdbcDatabaseInit(&database, &error));
+ if (error.release) error.release(&error);
ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
}
+TEST_F(DriverManager, UninitializedError) {
+ struct AdbcDatabase database;
+ struct AdbcError invalid_err;
+ std::memset(&database, 0, sizeof(database));
+
+ // Force junk into AdbcError going into calls to
+ // simulate API calls using uninitialized AdbcError structs
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err),
+ IsStatus(ADBC_STATUS_INVALID_STATE, &invalid_err));
+ if (invalid_err.release) invalid_err.release(&invalid_err);
Review Comment:
Do we need this `if`? I think that this must have `ADBC_STATUS_INVALID_STATE` error.
```suggestion
invalid_err.release(&invalid_err);
```
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -100,9 +149,12 @@ TEST_F(DriverManager, ConnectionOptions) {
ASSERT_THAT(AdbcConnectionNew(&connection, &error), IsOkStatus(&error));
ASSERT_THAT(AdbcConnectionSetOption(&connection, "foo", "bar", &error),
IsOkStatus(&error));
+
Review Comment:
```suggestion
```
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -100,9 +149,12 @@ TEST_F(DriverManager, ConnectionOptions) {
ASSERT_THAT(AdbcConnectionNew(&connection, &error), IsOkStatus(&error));
ASSERT_THAT(AdbcConnectionSetOption(&connection, "foo", "bar", &error),
IsOkStatus(&error));
+
ASSERT_EQ(ADBC_STATUS_NOT_IMPLEMENTED,
AdbcConnectionInit(&connection, &database, &error));
+
Review Comment:
```suggestion
```
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -83,9 +83,58 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
AdbcDatabaseSetOption(&database, "entrypoint", "ThisSymbolDoesNotExist", &error),
IsOkStatus(&error));
ASSERT_EQ(ADBC_STATUS_INTERNAL, AdbcDatabaseInit(&database, &error));
+ if (error.release) error.release(&error);
ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
}
+TEST_F(DriverManager, UninitializedError) {
+ struct AdbcDatabase database;
+ struct AdbcError invalid_err;
+ std::memset(&database, 0, sizeof(database));
+
+ // Force junk into AdbcError going into calls to
+ // simulate API calls using uninitialized AdbcError structs
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err),
+ IsStatus(ADBC_STATUS_INVALID_STATE, &invalid_err));
+ if (invalid_err.release) invalid_err.release(&invalid_err);
+
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseNew(&database, &error), IsOkStatus(&invalid_err));
Review Comment:
Is this correct?
```suggestion
ASSERT_THAT(AdbcDatabaseNew(&database, &invalid_err), IsOkStatus(&invalid_err));
```
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -83,9 +83,58 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
AdbcDatabaseSetOption(&database, "entrypoint", "ThisSymbolDoesNotExist", &error),
IsOkStatus(&error));
ASSERT_EQ(ADBC_STATUS_INTERNAL, AdbcDatabaseInit(&database, &error));
+ if (error.release) error.release(&error);
ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
}
+TEST_F(DriverManager, UninitializedError) {
+ struct AdbcDatabase database;
+ struct AdbcError invalid_err;
+ std::memset(&database, 0, sizeof(database));
+
+ // Force junk into AdbcError going into calls to
+ // simulate API calls using uninitialized AdbcError structs
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err),
+ IsStatus(ADBC_STATUS_INVALID_STATE, &invalid_err));
+ if (invalid_err.release) invalid_err.release(&invalid_err);
+
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseNew(&database, &error), IsOkStatus(&invalid_err));
+ ASSERT_THAT(
+ AdbcDatabaseSetOption(&database, "driver", "adbc_driver_sqlite", &invalid_err),
+ IsOkStatus(&invalid_err));
+
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err), IsOkStatus(&invalid_err));
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(
+ AdbcDatabaseSetOption(&database, "notavalidkey", "notavalidvalue", &invalid_err),
+ IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &invalid_err));
+ if (invalid_err.release) invalid_err.release(&invalid_err);
+
+ ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
+
+}
+
+TEST_F(DriverManager, NoAdbcError) {
+ struct AdbcDatabase database;
+ std::memset(&database, 0, sizeof(database));
+
+ // Make API calls with AdbcError=nullptr and ensure that
+ // nothing bad happens if the optional param is not passed in
+ EXPECT_EQ(AdbcDatabaseInit(&database, nullptr), ADBC_STATUS_INVALID_STATE);
+
+ EXPECT_EQ(AdbcDatabaseNew(&database, nullptr), ADBC_STATUS_OK);
+ EXPECT_EQ(AdbcDatabaseSetOption(&database, "driver", "adbc_driver_sqlite", nullptr),
+ ADBC_STATUS_OK);
+
+ EXPECT_EQ(AdbcDatabaseInit(&database, nullptr), ADBC_STATUS_OK);
+ EXPECT_EQ(AdbcDatabaseSetOption(&database, "notavalidkey", "notavalidvalue", nullptr),
+ ADBC_STATUS_NOT_IMPLEMENTED);
+ ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
+
Review Comment:
```suggestion
```
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -83,9 +83,58 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
AdbcDatabaseSetOption(&database, "entrypoint", "ThisSymbolDoesNotExist", &error),
IsOkStatus(&error));
ASSERT_EQ(ADBC_STATUS_INTERNAL, AdbcDatabaseInit(&database, &error));
+ if (error.release) error.release(&error);
ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
}
+TEST_F(DriverManager, UninitializedError) {
+ struct AdbcDatabase database;
+ struct AdbcError invalid_err;
+ std::memset(&database, 0, sizeof(database));
+
+ // Force junk into AdbcError going into calls to
+ // simulate API calls using uninitialized AdbcError structs
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err),
+ IsStatus(ADBC_STATUS_INVALID_STATE, &invalid_err));
+ if (invalid_err.release) invalid_err.release(&invalid_err);
+
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseNew(&database, &error), IsOkStatus(&invalid_err));
+ ASSERT_THAT(
+ AdbcDatabaseSetOption(&database, "driver", "adbc_driver_sqlite", &invalid_err),
+ IsOkStatus(&invalid_err));
+
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err), IsOkStatus(&invalid_err));
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(
+ AdbcDatabaseSetOption(&database, "notavalidkey", "notavalidvalue", &invalid_err),
+ IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &invalid_err));
+ if (invalid_err.release) invalid_err.release(&invalid_err);
Review Comment:
```suggestion
invalid_err.release(&invalid_err);
```
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -83,9 +83,58 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
AdbcDatabaseSetOption(&database, "entrypoint", "ThisSymbolDoesNotExist", &error),
IsOkStatus(&error));
ASSERT_EQ(ADBC_STATUS_INTERNAL, AdbcDatabaseInit(&database, &error));
+ if (error.release) error.release(&error);
ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
}
+TEST_F(DriverManager, UninitializedError) {
+ struct AdbcDatabase database;
+ struct AdbcError invalid_err;
Review Comment:
Why do we need this instead of existing `error`?
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -100,9 +149,12 @@ TEST_F(DriverManager, ConnectionOptions) {
ASSERT_THAT(AdbcConnectionNew(&connection, &error), IsOkStatus(&error));
ASSERT_THAT(AdbcConnectionSetOption(&connection, "foo", "bar", &error),
IsOkStatus(&error));
+
ASSERT_EQ(ADBC_STATUS_NOT_IMPLEMENTED,
AdbcConnectionInit(&connection, &database, &error));
+
ASSERT_THAT(error.message, ::testing::HasSubstr("Unknown connection option foo=bar"));
+ if (error.release) error.release(&error);
Review Comment:
```suggestion
error.release(&error);
```
and others...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] cdaudt commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "cdaudt (via GitHub)" <gi...@apache.org>.
cdaudt commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1492715036
>
> If it's documented in the header (thanks Kou for checking!) then we should stick to the existing behavior.
this patch doesn't require a change to the existing behaviour. If a caller does pass in a zero AdbcError then this patch doesn't harm that (just an added memset overhead which is a small price to pay for the protection to the API imo).
Do you mean you prefer I drop the patch?
The separate issue of driver-manager -> driver calls bypassing the zero'ed out AdbcError expectation on purpose seems like a bad way to get driver-manager & driver to collaborate on filling up an error structure. I can send a separate patch to try to address that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm closed pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm closed pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
URL: https://github.com/apache/arrow-adbc/pull/570
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1513944232
Ah, though there's still leaks in the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix (c/driver-manager): Protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1492567607
Finally, I think all current implementations assume caller will zero things, for all structures - not just the error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] cdaudt commented on a diff in pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "cdaudt (via GitHub)" <gi...@apache.org>.
cdaudt commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1154951154
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -57,6 +57,11 @@ void GetWinError(std::string* buffer) {
#endif // defined(_WIN32)
+// Struct initializers
+static void AdbcErrorInit(struct AdbcError* error) {
+ std::memset(error, 0, sizeof(*error));
Review Comment:
yes, you're right. So maybe a reason to keep the AdbcErrorInit function, and add a null check. I missed that comment in adbc.h - if that is the defined behaviour then the docs should probably replicate it to the individual API calls (DatabaseNew/ConnectionNew/StatementNew already call out that the Database/Connection/Statement parameters must be zero initialized).
As a defensive mechanism on the API whoever I do think it should be explicit in zeroing it, as in the current implementation if it is not zeroed then you get erratic behaviour (which is how I came across this - I was getting a segmentation violation because "message" had junk in it going in because I didn't realize I needed to initialize to zero, and that caused SetError to incorrectly call error->release() which also contained junk and caused the segmentation violation).
the need for the "error->release()" call by the API caller is something I had planned to bring up separately - seems like a bad idea to pass allocation back in an optional error field that is then responsibility of the caller to free (and I don't even see that mentioned in the documentation btw).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] cdaudt commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "cdaudt (via GitHub)" <gi...@apache.org>.
cdaudt commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1562318559
hi just a ping on this - are you okay to proceed with this PR? If so I'll rebase to latest and resubmit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #570: fix (c/driver-manager): Protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1154894112
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -597,6 +628,7 @@ AdbcStatusCode AdbcStatementSetSqlQuery(struct AdbcStatement* statement,
AdbcStatusCode AdbcStatementSetSubstraitPlan(struct AdbcStatement* statement,
const uint8_t* plan, size_t length,
struct AdbcError* error) {
+ AdbcErrorInit(error);
Review Comment:
Reset or something might be clearer? (Or is there a need for a function when it's only one line of code?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] cdaudt commented on pull request #570: fix (c/driver-manager): Protect against uninitialized AdbcError
Posted by "cdaudt (via GitHub)" <gi...@apache.org>.
cdaudt commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1492588817
>
Hi - thanks for the feedback. The api defines this field as '[out]' and thus the caller should not expect that anything passed into the API will survive. e.g. here: [https://arrow.apache.org/adbc/main/cpp/api/adbc.html#_CPPv421AdbcConnectionReleaseP14AdbcConnectionP9AdbcError](AdbcConnectionRelease)
But I do agree that while the driver manager is okay in that calls such as AdbcConnectionRelease are only called from the API clients, the same is not true in the actual drivers, where the same function is used both to provide the public API as well as the callback to the driver manager today. Will need to address that in equivalent changes to the drivers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1492722295
Ah, you're right. Then checking for null should be enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1513944871
So the real question is which footgun do we prefer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] kou commented on a diff in pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1170690027
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -86,6 +86,47 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
}
+TEST_F(DriverManager, UninitializedError) {
+ struct AdbcDatabase database;
+ struct AdbcError invalid_err;
+ std::memset(&database, 0, sizeof(database));
+
+ // Force junk into AdbcError going into calls to
+ // simulate API calls using uninitialized AdbcError structs
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err),
+ IsStatus(ADBC_STATUS_INVALID_STATE, &invalid_err));
Review Comment:
We need to call `invalid_err.release(&invalid_err)` after this.
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -86,6 +86,47 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
}
+TEST_F(DriverManager, UninitializedError) {
+ struct AdbcDatabase database;
+ struct AdbcError invalid_err;
+ std::memset(&database, 0, sizeof(database));
+
+ // Force junk into AdbcError going into calls to
+ // simulate API calls using uninitialized AdbcError structs
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err),
+ IsStatus(ADBC_STATUS_INVALID_STATE, &invalid_err));
+
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseNew(&database, &error), IsOkStatus(&invalid_err));
+ ASSERT_THAT(
+ AdbcDatabaseSetOption(&database, "driver", "adbc_driver_sqlite", &invalid_err),
+ IsOkStatus(&invalid_err));
+
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err), IsOkStatus(&invalid_err));
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(
+ AdbcDatabaseSetOption(&database, "notavalidkey", "notavalidvalue", &invalid_err),
+ IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &invalid_err));
Review Comment:
Ditto.
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -86,6 +86,47 @@ TEST_F(DriverManager, DatabaseCustomInitFunc) {
ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error));
}
+TEST_F(DriverManager, UninitializedError) {
+ struct AdbcDatabase database;
+ struct AdbcError invalid_err;
+ std::memset(&database, 0, sizeof(database));
+
+ // Force junk into AdbcError going into calls to
+ // simulate API calls using uninitialized AdbcError structs
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseInit(&database, &invalid_err),
+ IsStatus(ADBC_STATUS_INVALID_STATE, &invalid_err));
+
+ std::memset(&invalid_err, 0xff, sizeof(invalid_err));
+ ASSERT_THAT(AdbcDatabaseNew(&database, &error), IsOkStatus(&invalid_err));
+ ASSERT_THAT(
+ AdbcDatabaseSetOption(&database, "driver", "adbc_driver_sqlite", &invalid_err),
+ IsOkStatus(&invalid_err));
Review Comment:
Ditto.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1492706305
If it's documented in the header (thanks Kou for checking!) then we should stick to the existing behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] kou commented on a diff in pull request #570: fix(c/driver_manager): protect against uninitialized AdbcError
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#discussion_r1154924317
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -57,6 +57,11 @@ void GetWinError(std::string* buffer) {
#endif // defined(_WIN32)
+// Struct initializers
+static void AdbcErrorInit(struct AdbcError* error) {
+ std::memset(error, 0, sizeof(*error));
Review Comment:
We should add a `NULL` check because `error` is an optional out parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-adbc] lidavidm commented on pull request #570: fix (c/driver-manager): Protect against uninitialized AdbcError
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #570:
URL: https://github.com/apache/arrow-adbc/pull/570#issuecomment-1492567190
Thanks!
I'll note, some drivers deliberately append to an existing error, which we can't do with this. (Also, this means that you need to check and release the error before making further calls: probably good practice, but will also trip people up)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org