Cleanups from NCBI as of May 2024 #560

ucko · 2024-05-30T14:35:30Z

This PR contains three formally interdependent commits from #555; inasmuch as there's an overall theme, it's formal cleanups.

freddy77 · 2024-07-06T15:09:25Z

src/dblib/dbpivot.c

-		*s++ = strdup(string_value(pc));
+		*s++ = string_value(pc);


freddy77 · 2024-07-06T15:10:10Z

src/tds/challenge.c

-	p = strchr(user_name, '\\');
+	p = user_name ? strchr(user_name, '\\') : NULL;


this is useless, compiler bug?

Good point.

freddy77 · 2024-07-06T15:11:10Z

src/tds/challenge.c

+	if (p != NULL) {
+		user_name = p + 1;
+		user_name_len = strlen(user_name);
+	}


useless too, although in this case compiler can't much help, the username here has to have that kind of format

I was concerned that the (indirect) caller might have supplied an unsuitably formatted username, but a closer look reveals that tds7_send_auth checks for a little-endian UCS-2 backslash before kicking off the call chain that leads to make_ntlm_v2_hash.

freddy77 · 2024-07-06T15:14:18Z

src/tds/config.c

-		login->valid_configuration = 0;
+		if (login)
+			login->valid_configuration = 0;


login is used dozens of times in this function, we would have crash much sooner than here

The previous uses are all contingent on specific option values, so a (hypothetical!) call along the lines of

tds_parse_conf_section("unimplemented", value, NULL);

could otherwise crash right there, but I don't know why the analyzer singled that one line out.

freddy77 · 2024-07-06T15:16:47Z

src/tds/query.c

+	if (!buf) {
+		*out_len = 0;
 		return NULL;
+	}


caller should handle the error due to NULL and not use out_len... but won't hurt initializing it

freddy77 · 2024-07-06T15:17:44Z

src/tds/vstrbuild.c

@@ -156,7 +156,7 @@ tds_vstrbuild(char *buffer, int buflen, int *resultlen, const char *text, int te
 		case CALCPARAM:
 			switch (*text) {
 			case '!':
-				if (pnum <= tokcount) {


freddy77 · 2024-07-06T15:18:17Z

src/tds/challenge.c

+	if (client_data == NULL	 ||  client_data_len <= 0)
+		return NULL;
+


useless code

Good point. Perhaps more _len variables and arguments should be size_t or some other unsigned type for clarity, though.

freddy77 · 2024-07-06T15:21:50Z

src/odbc/convert_tds2sql.c

-	TDSICONV *conv = curcol->char_conv;
+	if (curcol == NULL) {
+		odbc_errs_add(&stmt->errs, "HY013", NULL);
+		return SQL_NULL_DATA;
+	}


curcol is never NULL here.

freddy77 · 2024-07-06T21:11:42Z

Merged part of "Formally address static analyzer concerns.".

About the test_assert.h: I suppose the reason is to be able to override assert macro using that header. Then why not using a different macro, kind of TDS_ASSERT or similar?

ucko · 2024-07-08T19:01:43Z

Thanks! As for test_assert.h, our version serves two purposes: forcing direct calls to assert (otherwise left stock) to take effect for the current compilation unit even in release mode (by undefining NDEBUG before including system assert.h) and, on Windows, arranging to suppress dialog boxes from crashes of any sort so that they don't stall automated test runs.

freddy77 · 2024-07-10T14:32:43Z

Thanks! As for test_assert.h, our version serves two purposes: forcing direct calls to assert (otherwise left stock) to take effect for the current compilation unit even in release mode (by undefining NDEBUG before including system assert.h) and, on Windows, arranging to suppress dialog boxes from crashes of any sort so that they don't stall automated test runs.

NDEBUG should be manually undefined in all tests. But it seems to me a good reason to have a different macro name then. It's confusing having a code using standard macro names but needed them to be no standard macros. Would something like this freddy77@bc1e07f make sense (beside the dumb commit message)?

ucko · 2024-07-10T18:40:35Z

Even with my additional adjustment in place, I wouldn't be looking to do anything to assert per se beyond always using the real (non-NDEBUG) implementation; I suppose we could have every test #include "common.h" first and every common.h (not just the ones replacements/unittests and utils/unittests would gain) #undef NDEBUG and perhaps also #include <assert.h> for convenience.

There's also the matter of centrally suppressing the dialog boxes Windows usually pops up when anything crashes (conditionally on a DIAG_SILENT_ABORT environment setting starting with Y or y). The C++ Toolkit header I've been arranging to use handles that by supplying a static function that conditionally performs the relevant tuneups and injecting a pointer to it in the .CRT$XIV (init level V) section, but in this case I suppose it might be more reasonable to supply a central function along those lines and have every test call it at the top of main. (In the context of C++, non-trivial initializers can run and potentially crash before main starts, calling for a more aggressive approach.)

freddy77 · 2024-07-11T13:57:55Z

You can call _set_error_mode and/or _set_abort_behavior to change assert/abort behaviour on Windows. How to make sure they are called before assert instead of using some weird MS internals (which won't work possibly using GCC or other compilers) that's another issue. One way would be to define assert and call a "empowered" _assert function which would do everything needed. Or define in common.c a main that calls a test_main to be define by each test. At this point I'm starting thinking that having some common generic test code to be reused by all components would be helpful. The code that reads the PWD file is duplicated quite a lot for instance.

ucko · 2024-07-12T17:55:15Z

Good idea. I'll want to make this library distinct from libcommon so that the unit tests for replacements and utils can use it too. Also, those tests (and some others) all work offline, and as such shouldn't care whether the PWD file exists, so the test main shouldn't insist on finding it itself. (This is not just a hypothetical situation -- the C++ Toolkit runs the online tests via a wrapper that confirms that they work properly when run against both Sybase and MS SQL servers by varying TDSPWDFILE.)

freddy77 · 2024-07-13T08:35:25Z

Good idea. I'll want to make this library distinct from libcommon so that the unit tests for replacements and utils can use it too. Also, those tests (and some others) all work offline, and as such shouldn't care whether the PWD file exists, so the test main shouldn't insist on finding it itself. (This is not just a hypothetical situation -- the C++ Toolkit runs the online tests via a wrapper that confirms that they work properly when run against both Sybase and MS SQL servers by varying TDSPWDFILE.)

I suppose by libcommon you mean the various libcommon generated by unittests directory. A more detailed idea is to have this new common library in utils/unittests providing utilities for all unittests directories. The history about directory and dependencies was that mainly odbc, ctlib and dblib all depended on tds while replacements was only to define portable implementation of some system functions (like strlcpy) not available in all systems. But lot of features provided by tds started to be protocol/database independent and some started to be added to replacements even if there were not really replacements. So the utils was born, to separate common functions not dealing with database, protocol or strict portability. The libcommon in unittests instead were born to avoid compiling common.c and other files for each test program and also to make CMake less of a burden to maintain.

It's not clear from your paragraph the part about offline tests and online tests. One part of the paragraphs says that "those tests (and some others) all work offline" while another that "C++ Toolkit runs the online tests via a wrapper". So apparently one sentence applies to some tests and latter to other ones.

ucko · 2024-07-15T15:58:01Z

To clarify:

I was referring to the libcommon from tds/unittests, aka libt_common, which sees a little use from outside that directory already -- ctlib and odbc both have all_types tests that use its tds_all_types. However, that function of course depends on libtds, making the library unsuitable for use by tests for replacements or utils, hence my comment that we will need to introduce a new internal library rather than extending that one.
Some tests (all replacements and utils tests, and several others) don't connect to any servers and as such shouldn't care whether a PWD file exists. The remainder are worth testing against both MS SQL Server and Sybase ASE in environments where both are available. As such, the C++ Toolkit doesn't establish a central PWD file, but rather arranges to run the tests that do connect via a wrapper script that varies TDSPWDFILE and fails if any invocation does, and those that don't just once each to save time.

* Conditionally stub out the tests that require threading (as of 1.4.12) and don't yet have such logic. * include/freetds/thread.h: Never try to add extra checks around stubs. Signed-off-by: Aaron M. Ucko <[email protected]>

* Introduce a <freetds/utils/test_base.h> that for now simply ensures that assertions in test-specific code will always be active and pulls in <config.h>. (Tests that use assertions are still responsible for including <assert.h> themselves, but <config.h> is generally best included as early as possible.) * Start every common.h with #include <freetds/utils/test_base.h> and src/tds/unittests/{parsing,tls}.c with #include "common.h". (Every other unit test with an accompanying common.h already had that form.) * Don't directly undefine NDEBUG or include <config.h> anywhere in */src/unittests. Signed-off-by: Aaron M. Ucko <[email protected]>

Define it in the recently introduced <freetds/utils/test_base.h> (which now includes <freetds/macros.h> accordingly) as int main(int argc TDS_UNUSED, char **argv TDS_UNUSED) so that main can have the same signature in tests that ignore any arguments as in tests that consult arguments without yielding any warnings. Use this macro for all tests to pave the way for introducing a central main wrapper around what will then be a test-specific test_main. Signed-off-by: Aaron M. Ucko <[email protected]>

Consult the environment variable DIAG_SILENT_ABORT (if present), looking for a value starting with Y or y. To that end, adjust TEST_MAIN to call test-specific entry points test_main and incorporate a new tds_test_base library defining a wrapper main that, on Windows, first suppresses any possible source of crash dialog boxes. Signed-off-by: Aaron M. Ucko <[email protected]>

Extend the recently introduced tds_test_base library to supply a new COMMON_PWD data structure (mostly based on ctlib tests'), an instance thereof (the only one in practice), functions for working with it, and the default (relative) path to try. Adjust everything else to use this setup rather than a local variant thereof. Let historical variable names such as SERVER continue to work within dblib tests because all online dblib tests need to consult them, but explicitly go through the instance elsewhere, including dblib tests' common.c. Signed-off-by: Aaron M. Ucko <[email protected]>

* Add more explicit sanity checks and initializers. * vstrbuild.c (tds_vstrbuild): Use unsigned int for counts that will never be negative. * rpc_ct_setparam.c (ex_display_results): Avoid memory leaks in some error cases. Signed-off-by: Aaron M. Ucko <[email protected]>

freddy77 reviewed Jul 6, 2024

View reviewed changes

ucko added 6 commits July 17, 2024 16:41

Fix compilation errors in single-threaded builds.

e8f23eb

* Conditionally stub out the tests that require threading (as of 1.4.12) and don't yet have such logic. * include/freetds/thread.h: Never try to add extra checks around stubs. Signed-off-by: Aaron M. Ucko <[email protected]>

ucko force-pushed the ncbi-2024-05-cleanups branch from d6ce7b0 to bbe2948 Compare July 17, 2024 20:44

ucko mentioned this pull request Jul 18, 2024

Inform CMake of some accidentally Autotools-only tests. #601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanups from NCBI as of May 2024 #560

Cleanups from NCBI as of May 2024 #560

ucko commented May 30, 2024

freddy77 Jul 6, 2024

freddy77 Jul 6, 2024

ucko Jul 8, 2024

freddy77 Jul 6, 2024

ucko Jul 8, 2024

freddy77 Jul 6, 2024

ucko Jul 8, 2024

freddy77 Jul 6, 2024

freddy77 Jul 6, 2024

freddy77 Jul 6, 2024

ucko Jul 8, 2024

freddy77 Jul 6, 2024

freddy77 commented Jul 6, 2024

ucko commented Jul 8, 2024 •

edited

Loading

freddy77 commented Jul 10, 2024

ucko commented Jul 10, 2024

freddy77 commented Jul 11, 2024

ucko commented Jul 12, 2024

freddy77 commented Jul 13, 2024

ucko commented Jul 15, 2024

		p = strchr(user_name, '\\');
		p = user_name ? strchr(user_name, '\\') : NULL;

		if (client_data == NULL \|\| client_data_len <= 0)
		return NULL;

Cleanups from NCBI as of May 2024 #560

Are you sure you want to change the base?

Cleanups from NCBI as of May 2024 #560

Conversation

ucko commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddy77 commented Jul 6, 2024

ucko commented Jul 8, 2024 • edited Loading

freddy77 commented Jul 10, 2024

ucko commented Jul 10, 2024

freddy77 commented Jul 11, 2024

ucko commented Jul 12, 2024

freddy77 commented Jul 13, 2024

ucko commented Jul 15, 2024

ucko commented Jul 8, 2024 •

edited

Loading