Augment rand argument parsing to allow scaling

[nhorman]

Extend opts parsing to parse long long values, and use it to have openssl rand accept requests for more than 2^32-1 bytes of data

Fixes #22622

[mspncp]

As mentioned elsewhere (#22622 (comment)), a size_t would be my preferred choice.

Suggested change

	/* Parse a long long, put it into *result; return 0 on failure, else 1. */
	int opt_long_long(const char *value, long long *result)
	/* Parse a size argument with an optional size suffix (e.g., 1024, 1k, 30M), put it into *result; return 0 on failure, else 1. */
	int opt_size(const char *value, size_t *result)

[paulidale]

It's signed here, so size_t wouldn't be correct. int64_t is the one to change to.

[nhorman]

I think if we allow for scaling (i.e. 256k, 800g, etc), opt_long is still perfectly acceptable no?

[mspncp]

I guess you meant to write openssl rand, not opt rand (commit message & pr title)?

allow openssl rand to accept larger data requests

[mspncp]

Nice work. Some suggestions for minor enhancements.

[mspncp]

I'd make it case insensitive and disallow multiple suffixes:

Suggested change

	if (!isdigit((int)(argv[0][factoridx]))) {
	switch(argv[0][factoridx]) {
	case 'k':
	scale = 10;
	break;
	case 'm':
	scale = 20;
	break;
	case 'g':
	scale = 30;
	break;
	case 't':
	scale = 40;
	default:
	goto opthelp;
	}
	break;
	}
	factoridx++;
	if (!isdigit((int)(argv[0][factoridx]))) {
	if (shift != 0)
	goto opthelp;
	switch(argv[0][factoridx]) {
	case 'k':
	case 'K':
	shift = 10;
	break;
	case 'm':
	case 'M':
	shift = 20;
	break;
	case 'g':
	case 'G':
	shift = 30;
	break;
	case 't':
	case 'T':
	shift = 40;
	default:
	goto opthelp;
	}
	break;
	}
	factoridx++;

[paulidale]

Why not do as dd(1) does for size suffixes?

N and BYTES may be followed by the following multiplicative suffixes: c=1, w=2, b=512, kB=1000, K=1024, MB=10001000, M=10241024, xM=M, GB=100010001000, G=102410241024, and so on for T, P, E, Z, Y. Bi‐ nary prefixes can be used, too: KiB=K, MiB=M, and so on.

Also noting that m and M are distinct SI prefixes so case insensitivity could be confusing.

[mspncp]

This seams to be consistent with what head(1) uses

NUM may have a multiplier suffix: b 512, kB 1000, K 1024, MB 1000*1000, M 1024*1024, GB 1000*1000*1000, G 1024*1024*1024, and so on for T, P, E, Z, Y.  Binary prefixes can be used, too: KiB=K, MiB=M, and so on.

[mspncp]

A general remark: just doing a goto opthelp; without printing any useful error message will create a lot of frustration for the app user, because he will only see the same usage message over and over again until he guesses what he messed up. (I know this from my own experience).

[tom-cosgrove-arm]

@nhorman Do let us know when you want a full review (this is still a draft PR)

[nhorman]

@tom-cosgrove-arm ack, I think its ready now

[mspncp]

I very much appreciate that you try to avoid force pushes, but now is a good time to do an interactive rebase in order to cleanup your history. If you do it without moving the merge-base (git rebase -i --keep-base ...) it will be easy for the reviewers to verify that you didn't introduce any tree changes, using GitHub's force-push diff.

Please note, that the commit message is outdated, the terabytes are still missing.

Augment rand argument parsing to allow scaling

Instead of just accepting a number of bytes, allows openssl rand to
accept a k|m|g suffix to scale to kbytes/mbytes/gbytes

Fixes #22622

I'd also appreciate if you'd change your PR title to match the subject line of the commit: Augment rand argument parsing to allow scaling

[mspncp]

And be warned: I will insist on strict parsing of the suffixes (with a meaningful error message). Or do you really want to get a bug report because someone called

openssl rand 10km

and didn't get 10 kilometres of random output? 😉

[mspncp]

At least in the commit message and the documentation (usage, manpage) we should use the correct official abbreviations KiB / MiB / GiB / TiB for clarity:

accept a [k|m|g|t] suffix to scale to KiB/MiB/GiB/TiB

(BTW: I noticed that @jirka-h forgot to request support for PiB) 😉

[mspncp]

Fun Trivia: NIST has a really neat page about Prefixes for binary multiples, which applies in particular to the section about the historical context.

[jirka-h]

(BTW: I noticed that @jirka-h forgot to request support for PiB) 😉

😄 Check man head - it supports prefixes all the way up to YiB (2^80). That's future ready:-)

Joking aside: SP 800-90C limits DRBG output to 2^64 bits per instantiation. We can go up to EiB (2^60). I have been testing RNGs (batteries of statistical tests) in the past (~6 years ago), going up to 128 TiB. With today's HW, reaching PiB is no problem - openssl rand achieves rate of around 1.5GiB/s on my laptop, meaning you can generate 1PiB in just ~6 days on decent HW.

[t8m]

CI is relevant, other than that this looks good.

[tom-cosgrove-arm]

LGTM

CI failing

[nhorman]

Its the intermittent sslapi failure

[paulidale]

I don't see a need for stopping the timer or getting re-reviews for the final case change in the comment.

[tom-cosgrove-arm]

My approval still stands

[tom-cosgrove-arm]

I think full approval is done, so updating labels

[mspncp]

I don't see a need for stopping the timer or getting re-reviews for the final case change in the comment.

It's more than just a simple case change: As I pointed out three days ago the terabytes are not mentioned yet, and I'd like to see the official abbreviations.

The commit message is part of the review, so I'd like to get to see it before the pull request can be approved. @nhorman could either autosquash the pull request (without moving the merge base) and edit the commit message, or alternatively he could post the intended commit message here, e.g., like this:

Augment rand argument parsing to allow scaling

Instead of just accepting a number of bytes, allows openssl rand to
accept a K|M|G|T suffix to scale to KiB/MiB/GiB/TiB.

Fixes #22622

Considering how picky we sometimes are about trivial whitespace errors in the code, I really don't understand why we don't care in the same way about the correctness and quality of the commit messages.

Also, I requested strict parsing, i.e. we shouldn't allow multiple suffixes with undefined behaviour. It was already the second time I asked for it, but up to now my request has simply been ignored.

[t8m]

Considering how picky we sometimes are about trivial whitespace errors in the code, I really don't understand why we don't care in the same way about the correctness and quality of the commit messages.

@mspncp Please bring up this topic on the next OTC meeting. We aren't picky about commit messages (maybe we should be) but it seems to me unfair to be particularly picky on this particular PR.

Also, I requested strict parsing, i.e. we shouldn't allow multiple suffixes with undefined behaviour. It was already the second time I asked for it, but up to now my request has simply been ignored.

But if I understand the current code right it does NOT allow multiple suffixes. That's one of the things I particularly looked at when reviewing this PR.

If you really really want to block this PR in this form from merging, please put an OTC hold there. Otherwise IMO this is good to go in once the 24h timeout expires (with possible commit message adjustments that can be done during merging).

[mspncp]

Considering how picky we sometimes are about trivial whitespace errors in the code, I really don't understand why we don't care in the same way about the correctness and quality of the commit messages.

@mspncp Please bring up this topic on the next OTC meeting. We aren't picky about commit messages (maybe we should be) but it seems to me unfair to be particularly picky on this particular PR.

Fair point, you are right.

But if I understand the current code right it does NOT allow multiple suffixes. That's one of the things I particularly looked at when reviewing this PR.

To be honest, I only looked at the source code and didn't compile and test it. But it looks to me like the app loops over the suffixes and every suffix overwrites the previous one, such that 10KM effectively becomes 10M, right? A simple check for shift != 0 before the switch statement with an error message about multiple suffixes would have prevented this.

openssl/apps/rand.c

Lines 125 to 133 in b273c3d

	while (argv[0][factoridx]) {
	if (!isdigit((int)(argv[0][factoridx]))) {
	switch(argv[0][factoridx]) {
	case 'K':
	shift = 10;
	break;
	case 'M':
	shift = 20;
	break;

If you really really want to block this PR in this form from merging, please put an OTC hold there. Otherwise IMO this is good to go in once the 24h timeout expires (with possible commit message adjustments that can be done during merging).

No, I'm not blocking it, because overall I really like the change. It's ok for me if people have different opinions and change requests can be debated and/or rejected. But the fact that my requests were simply ignored is what annoyed me a little bit and this was the reason for my reply. Sorry @nhorman if my last reply was too harsh, it wasn't meant personal.

[nhorman]

To be honest, I only looked at the source code and didn't compile and test it. But it looks to me like the app loops over the suffixes and every suffix overwrites the previous one, such that 10KM effectively becomes 10M, right?
@mspncp Thats not what it does:

            while (argv[0][factoridx]) {
                if (!isdigit((int)(argv[0][factoridx]))) {
                    switch(argv[0][factoridx]) {
                    case 'K':
                        shift = 10;
                        break;
                    case 'M':
                        shift = 20;
                        break;
                    case 'G':
                        shift = 30;
                        break;
                    case 'T':
                        shift = 40;
                        break;
                    default:
                        BIO_printf(bio_err, "Invalid size suffix %s\n",
                                   &argv[0][factoridx]);
                        goto opthelp;
                    }
                    break;   <=== Outside the switch statement
                }
                factoridx++;
            }

We scan for non-digit characters, and on the first one we find, we parse it, set the shift or error out accordingly, break from the case statement, then break from the while loop.

The algorithm considers only the first non digit suffix. So:

openssl rand 256MetersPerSecondSquared

translates to:

openssl rand 256M

Which seems to me is ok. At the very least I would get a chuckle out of someone opening a bug indicating that openssl has poor acceleration.

[mspncp]

Oh, thanks for the clarification! Indeed, I overlooked this little break:

                    break;   <=== Outside the switch statement

[openssl-machine]

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

[t8m]

Merged to the master branch. Thank you.

[jirka-h]

THANK YOU, EVERYBODY!