fix: Alter table issue

[abmusse]

Fixes #22

Turns out that custom min and max macros here were buggy.

db2iengine/db2i_global.h

Lines 77 to 78 in 7339d24

	#define min(a,b) ((long long)(a)>(long long)(b) ? (b) : (a))
	#define max(a,b) ((long long)(a)<(long long)(b) ? (b) : (a))

In this situation min it returned a very large value that got passed into memcpy as num_bytes causing the failure.

db2iengine/db2i_file.cc

Line 380 in 7339d24

memcpy(temp, part1, min(outlen, part2 - part1));

In this PR we remove using the min and max macros and instead use std::min and std::max.
This also required casting some variables to the appropriate types to work with std::min and std::max.

CC @kadler

[kadler]

Looks like these should use num_bytes, no?

[abmusse]

yes. Thanks for catching that

[kadler]

I think a temp variable would be useful here. Also, looks like maybe the original code had a bug since the memset references part1 and part2, but the other two lines use part3 and part4.

[abmusse]

Created temp varialble of num_bytes2 to store static_cast<size_t>(part4-part3)
Yes looks like bug as they memset with num_bytes of part2-part1 and later memcpy with num_bytes of part4-part3

[kadler]

I'm not 100% certain it is a bug, since why would you need to memset if you're going to memcpy over the same amount of data? Of course, it could just be overly defensive code.

[kadler]

I would probably change uint o to size_t o instead of the static_cast.

[abmusse]

Changed this to size_t o

https://github.com/zendtech/db2iengine/pull/23/files#diff-aa454e414a26f204ea0e1edcc31dbc9714afe333766b3d2388e6af82a41e01f0R85

[kadler]

With the change of DEFAULT_MAX_ROWS_TO_BUFFER to an ha_rows, this static_cast should be unnecessary.

[abmusse]

Yes fixed this to remove the unnecessary cast

https://github.com/zendtech/db2iengine/pull/23/files#diff-e0e69605b45f256c19b7588c8c2d3d9b30bed1f1cd54f6fd742807a4d51e61e3R2794

[kadler]

MAX_DB2_FILENAME_LENGTH should probably be a size_t and likely lastDupKeyNameLen too, but this seems fine for now.

[kadler]

Suggested change

	size_t num_bytes = std::min(outlen, static_cast<size_t>(part2 - part1));
	memcpy(temp, part1, std::min(outlen, num_bytes));
	temp[std::min(outlen-1, num_bytes)] = 0;
	size_t num_bytes = std::min(outlen-1, static_cast<size_t>(part2 - part1));
	memcpy(temp, part1, num_bytes);
	temp[num_bytes] = 0;

In the memcpy line, outlen has already been used to initialize num_bytes, no need to min against it again. In fact, once the min has been set there's no need to do any further calculations. The only change needed is to reserve one byte from the buffer length for the null terminator.

[kadler]

Suggested change

	return std::max(indexReadSizeEstimates[index], i);
	return std::max(indexReadSizeEstimates[index], ha_rows{1});

Don't need the extraneous variable definition, without having the verbose static_cast syntax

[kadler]

Suggested change

memset(temp, 0, std::min(outlen, num_bytes));

This isn't needed since we're going to overwrite it anyway.

[kadler]

Same as above. Also, we never use num_bytes from above again, so we could re-use it here instead of making num_bytes2.

Might be worth wrapping this code in to a function / lambda which can be called. Essentially what's being done is similar to strlcpy, eg.

// like strclpy, but takes srclen parameter too
auto strlcpy2 = [](char* dst, size_t size, const char* src, size_t srclen) {
  size = std::min(size-1, srclen);
  memcpy(dst, src, size);
  dst[size] = 0;
  return srclen;
}

That way we don't need the num_bytes variable around at all and the callsite is simplified to:

strlcpy2(tmp, outlen, part3, part4-part3);

Of course, as I look further, the only reason we are doing this copy in the first place is to extract a substring from in that is null-terminated so we can pass that string to smartFilenameToTableName. If smartFilenameToTableName took a "source length" parameter (instead of just assuming null-termination), we wouldn't even need to make any copies at all and most of this code would go away.

Maybe something like:

size_t db2i_table::smartFilenameToTableName(const char *in, size_t inlen, char* out, size_t outlen) {
 // copy old code here and adjust, using inlen
}

size_t db2i_table::smartFilenameToTableName(const char *in, char* out, size_t outlen) {
  return smartFilenameToTableName(in, strlen(in), out, outlen);
}
...

accumLen += smartFilenameToTableName(part3, part4-part3, &out[accumLen], outlen-accumLen);

Of course, now I'm really off in to the weeds for this change.

[kadler]

We talked about this yesterday, the simplest "fix" for this is to just wrap these in a new scope:

{
  size_t num_bytes = std::min(outlen, static_cast<size_t>(part2 - part1));
  memcpy(temp, part1, std::min(outlen, num_bytes));
  temp[std::min(outlen-1, num_bytes)] = 0;
}

This way num_bytes cannot be used outside the scope and we can re-use the num_bytes name for each without contents of the variable accidentally affecting something else.

[abmusse]

Updated