t/helper/test-tool: implement 'sha1-unsafe' helper

t/helper/test-hash.c +1 -1

-int cmd_hash_impl(int ac, const char **av, int algo)
+int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)
 {
 	git_hash_ctx ctx;
 	unsigned char hash[GIT_MAX_HEXSZ];

t/helper/test-hash.c +0 -1

 	while (1) {
 		ssize_t sz, this_sz;

@@ -46,9 +49,15 @@ int cmd_hash_impl(int ac, const char **av, int algo)
 		}
 		if (this_sz == 0)
 			break;
-		algop->update_fn(&ctx, buffer, this_sz);

Quoted 6 lines · Taylor Blau

t/helper/test-hash.c +1 -1

-int cmd_hash_impl(int ac, const char **av, int algo)
+int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)
 {
 	git_hash_ctx ctx;
 	unsigned char hash[GIT_MAX_HEXSZ];

t/helper/test-hash.c int cmd_hash_impl(int ac, const char **av, int algo) · line 33

Quoted 8 lines

t/helper/test-hash.c +4 -1

@@ -27,7 +27,10 @@ int cmd_hash_impl(int ac, const char **av, int algo)
 			die("OOPS");
 	}
 
-	algop->init_fn(&ctx);
+	if (unsafe)
+		algop->unsafe_init_fn(&ctx);
+	else
+		algop->init_fn(&ctx);

It may be just me, and it would not matter all that much within the context of the project because this is merely a test helper, but giving a pair of init/unsafe_init methods to algop looks unnerving. It gives an impression that every design of hash algorithm must come with normal and unsafe variant, which is not what you want to say.

I would have expected that there are different algorighm instances, and one of them would be "unsafe SHA-1", among "normal SHA-1" and "SHA-256" (as the last one would not even have unsafe_init_fn method), and the callers that want to measure the performance of each algorithm would simply pick one of these instances and go through the usual "init", "update", "final" flow, regardless of the "unsafe" ness of the algorithm.

IOW, ...

Quoted 11 lines

t/helper/test-hash.c +4 -1

 	while (1) {
 		ssize_t sz, this_sz;

@@ -46,9 +49,15 @@ int cmd_hash_impl(int ac, const char **av, int algo)
 		}
 		if (this_sz == 0)
 			break;
-		algop->update_fn(&ctx, buffer, this_sz);
+		if (unsafe)
+			algop->unsafe_update_fn(&ctx, buffer, this_sz);
+		else
+			algop->update_fn(&ctx, buffer, this_sz);

t/helper/test-hash.c int cmd_hash_impl(int ac, const char **av, int algo) · line 60

Quoted 6 lines

t/helper/test-hash.c +4 -1

 	}
-	algop->final_fn(hash, &ctx);
+	if (unsafe)
+		algop->unsafe_final_fn(hash, &ctx);
+	else
+		algop->final_fn(hash, &ctx);

... I didn't expect any of these "if (unsafe) .. else .." switches.

Quoted 7 lines

I would have expected that there are different algorighm instances, and one of them would be "unsafe SHA-1", among "normal SHA-1" and "SHA-256" (as the last one would not even have unsafe_init_fn method), and the callers that want to measure the performance of each algorithm would simply pick one of these instances and go through the usual "init", "update", "final" flow, regardless of the "unsafe" ness of the algorithm.

[...]

Quoted 1 line

... I didn't expect any of these "if (unsafe) .. else .." switches.

I don't think we can remove those, and here's why. Certainly Taylor can correct me if I'm off base, though.

In the normal case, our hash struct is a union, and different implementations can have a different layout. A SHA-1 implementation will usually keep track of a 64-bit size, 5 32-bit words, and up to 64 bytes of data that might need to be processed. Maybe SHA-1-DC, our safe implementation, stores the size first, but our unsafe implementation is OpenSSL and it stores the 5 hash words first, or whatever.

If we use the same update and final functions, we'll end up with incorrect data because we'll have initialized the union contents with data for one implementation but are trying to update or finalize it with different data, which in the very best case will just produce broken results, and might just cause a segfault (if one of the implementations stores a pointer instead of storing the data in the union).

We could certainly adopt a different hash algorithm structure for safe and unsafe code, but we have a lot of places that assume that there's just one structure per algorithm. It would require a substantial amount of refactoring to remove those assumptions and deal with the fact that we now have two SHA-1 implementations. It would also be tricky to deal with the fact that for SHA-1, we might use the safe or unsafe algorithm, but for SHA-256, there's only one algorithm structure and we need to use it for both.

So I think you wouldn't want to allocate an enum or a slot in the hash_algos array, because it is not really an independent algorithm. But I think it _could_ work as a separate struct that the caller derives from the main hash algorithm. For example, imagine that the git_hash_algo struct didn't have unsafe_init_fn, etc, but instead had:

struct git_hash_algo *unsafe_implementation;

with a matching accessor like:

struct git_hash_algo *unsafe_hash_algo(struct git_hash_algo *algo)

  {
	/* if we have a faster "unsafe" implementation, use that */
	if (algo->unsafe_implementation)
		return algo->unsafe_implementation;
	/* otherwise just use the default one */
	return algo;

}

And then csum-file.c, rather than calling:

  the_hash_algo->unsafe_init_fn(...);
  ...
  the_hash_algo->unsafe_final_fn(...);

would do this:

  struct git_hash_algo *algo = unsafe_hash_algo(the_hash_algo);
  algo->init_fn(...);
  ...
  algo->final_fn(...);

And likewise this test helper would have a single conditional at the start:

if (unsafe)

	algo = unsafe_hash_algo(algo);

and the rest of the code would just use that.

All that said, I do not think it buys us anything for "real" code. There the decision between safe/unsafe is in the context of how we are using it, and not based on some conditional. So while the code in this test helper is ugly, I don't think we'd ever write anything like that for real. So it may not be worth the effort to refactor into a more virtual object-oriented way.

Ah, yes, I think that approach would be simpler and nicer to work with than a separate entry in the hash_algos array. We do, however, have some places that assume that a struct git_hash_algo * points into the hash_algos array (notably hash_algo_by_ptr), so we'd have to adjust for that, move the function pointers out into their own struct which we'd use for unsafe_hash_algo, or be careful never to call the relevant functions on our special git_hash_algo struct.

Yeah, I don't have a strong opinion one way or the other. I think, with the limitation I mentioned above, it would probably require a decent amount of refactoring if we took a different approach, and I'm fine with going with Taylor's current approach unless he wants to do that refactoring (in which case, great).

Me too. If we were fixing something ugly or error-prone that we expected to come up in real code, it might be worth it. But it's probably not for trying to accommodate a one-off test helper.

Quoted 32 lines · Jeff King

So I think you wouldn't want to allocate an enum or a slot in the hash_algos array, because it is not really an independent algorithm. But I think it _could_ work as a separate struct that the caller derives from the main hash algorithm. For example, imagine that the git_hash_algo struct didn't have unsafe_init_fn, etc, but instead had:

struct git_hash_algo *unsafe_implementation;

with a matching accessor like:

struct git_hash_algo *unsafe_hash_algo(struct git_hash_algo *algo)

  {
	/* if we have a faster "unsafe" implementation, use that */
	if (algo->unsafe_implementation)
		return algo->unsafe_implementation;
	/* otherwise just use the default one */
	return algo;

}

And then csum-file.c, rather than calling:

  the_hash_algo->unsafe_init_fn(...);
  ...
  the_hash_algo->unsafe_final_fn(...);

would do this:

  struct git_hash_algo *algo = unsafe_hash_algo(the_hash_algo);
  algo->init_fn(...);
  ...
  algo->final_fn(...);

Quoted 7 lines

And likewise this test helper would have a single conditional at the start:

if (unsafe)

	algo = unsafe_hash_algo(algo);

and the rest of the code would just use that.

Ah, yes, I think that approach would be simpler and nicer to work with than a separate entry in the hash_algos array. We do, however, have some places that assume that a struct git_hash_algo * points into the hash_algos array (notably hash_algo_by_ptr), so we'd have to adjust for that, move the function pointers out into their own struct which we'd use for unsafe_hash_algo, or be careful never to call the relevant functions on our special git_hash_algo struct.

Quoted 6 lines

All that said, I do not think it buys us anything for "real" code. There the decision between safe/unsafe is in the context of how we are using it, and not based on some conditional. So while the code in this test helper is ugly, I don't think we'd ever write anything like that for real. So it may not be worth the effort to refactor into a more virtual object-oriented way.

Yeah, I don't have a strong opinion one way or the other. I think, with the limitation I mentioned above, it would probably require a decent amount of refactoring if we took a different approach, and I'm fine with going with Taylor's current approach unless he wants to do that refactoring (in which case, great).

Quoted 7 lines

Quoted 6 lines

All that said, I do not think it buys us anything for "real" code. There the decision between safe/unsafe is in the context of how we are using it, and not based on some conditional. So while the code in this test helper is ugly, I don't think we'd ever write anything like that for real. So it may not be worth the effort to refactor into a more virtual object-oriented way.

Quoted 5 lines

Yeah, I don't have a strong opinion one way or the other. I think, with the limitation I mentioned above, it would probably require a decent amount of refactoring if we took a different approach, and I'm fine with going with Taylor's current approach unless he wants to do that refactoring (in which case, great).

I think it does buy you something for real code, which is that you don't have to remember to consistently call the unsafe_ variants of all of the various function pointers.

For instance, if you do

    the_hash_algo->unsafe_init_fn(...);

early on, and then later on by mistake write:

    the_hash_algo->update_fn(...);

Then your code is broken and will (as brian said) either in the best case produce wrong results, or likely segfault.

So in that sense it is worth doing as it avoids the caller from having to keep track of what "mode" they are using the_hash_algo in. But let's take it up later, I think.

Quoted 5 lines

Yeah, I don't have a strong opinion one way or the other. I think, with the limitation I mentioned above, it would probably require a decent amount of refactoring if we took a different approach, and I'm fine with going with Taylor's current approach unless he wants to do that refactoring (in which case, great).

I think it does buy you something for real code, which is that you don't have to remember to consistently call the unsafe_ variants of all of the various function pointers.

For instance, if you do

    the_hash_algo->unsafe_init_fn(...);

early on, and then later on by mistake write:

    the_hash_algo->update_fn(...);

Then your code is broken and will (as brian said) either in the best case produce wrong results, or likely segfault.

This series implements a new 'sha1-unsafe' test helper, similar to 't/helper/test-tool sha1'.

I have found such a helper to be really handy when debugging the new SHA1_UNSAFE build knobs, e.g., to easily compare the performance of the safe versus unsafe routines, different unsafe variants, etc.

The first patch prepares us by setting up the existing cmd_hash_impl() function to be able to conditionally use the unsafe variant. The final patch introduces a new 'sha1-unsafe' test helper which calls the new variant.

I think this is a useful thing to have, and I didn't see anything wrong in the implementation. I did notice some oddities that existed before your series:

1. Why do we have "test-tool sha256" at all? Nobody in the test suite calls it. It feels like the whole test-sha1/sha256/hash split is overly complicated. A single "test-tool hash" seems like it would be simpler, and it could take an "--algo" parameter (and an "--unsafe" one). I guess in the end we end up with the same options ,but the proliferation of top-level test-tool commands seems ugly to me (likewise "sha1_is_sha1dc").

Quoted 11 lines · Jeff King

I think this is a useful thing to have, and I didn't see anything wrong in the implementation. I did notice some oddities that existed before your series:

1. Why do we have "test-tool sha256" at all? Nobody in the test suite calls it. It feels like the whole test-sha1/sha256/hash split is overly complicated. A single "test-tool hash" seems like it would be simpler, and it could take an "--algo" parameter (and an "--unsafe" one). I guess in the end we end up with the same options ,but the proliferation of top-level test-tool commands seems ugly to me (likewise "sha1_is_sha1dc").

I think we do in pack_trailer in t/lib-pack.sh, but not in a greppable way:

  # Compute and append pack trailer to "$1"
  pack_trailer () {
  	test-tool $(test_oid algo) -b <"$1" >trailer.tmp &&
  	cat trailer.tmp >>"$1" &&
  	rm -f trailer.tmp
  }

I think a single helper with --algo and --unsafe parameters would also be fine and would probably be a little more tidy, as you mention.

Quoted 6 lines

  # Compute and append pack trailer to "$1"
  pack_trailer () {
  	test-tool $(test_oid algo) -b <"$1" >trailer.tmp &&
  	cat trailer.tmp >>"$1" &&
  	rm -f trailer.tmp
  }

Nice find. I think that it may be worth writing this as:

    case "$(test_oid algo)" in
    sha1)
        test-tool sha1 -b <"$1" >trailer.tmp
        ;;
    sha256)
        test-tool sha256 -b <"$1" >trailer.tmp
        ;;
    *)
        echo >&2 "unknown algorithm: $(test_oid algo)"
        exit 1
        ;;
    esac

Quoted 7 lines

1. Why do we have "test-tool sha256" at all? Nobody in the test suite calls it. It feels like the whole test-sha1/sha256/hash split is overly complicated. A single "test-tool hash" seems like it would be simpler, and it could take an "--algo" parameter (and an "--unsafe" one). I guess in the end we end up with the same options ,but the proliferation of top-level test-tool commands seems ugly to me (likewise "sha1_is_sha1dc").

I think we do in pack_trailer in t/lib-pack.sh, but not in a greppable way:

  # Compute and append pack trailer to "$1"
  pack_trailer () {
  	test-tool $(test_oid algo) -b <"$1" >trailer.tmp &&
  	cat trailer.tmp >>"$1" &&
  	rm -f trailer.tmp
  }

When you posed the question above, I wasn't sure why I added this
functionality: was it just to test my SHA-256 implementation adequately or did it have some actual utility in the testsuite? However, I knew if it didn't appear straightforwardly in git grep, any uses would involve test_oid, and boom, I was right.

I think a single helper with --algo and --unsafe parameters would also be fine and would probably be a little more tidy, as you mention.

2. You modified test-sha1.sh, but I've wondered if we should just delete that script. It is not ever invoked in the test suite AFAIK. If we want correctness tests, they should go into a real t[0-9]*.sh script (though one imagines we exercise the sha1 code quite a lot in the rest of the tests). And it starts with a weird ad-hoc performance test that doesn't really tell us much. A t/perf test would be better (if it is even worth measuring at all).

So I dunno. None of those are the fault of your series, but it is piling on yet another test-tool command.

Quoted 2 lines

So I dunno. None of those are the fault of your series, but it is piling on yet another test-tool command.

Yeah, I think there was a fair amount of interesting discussion about possible alternatives in this thread, which I am appreciative of.

I think that if nobody has strong feelings or issues with the current implementation to add the sha1-unsafe test-tool, that we should do so and take these patches.

In the future we can consider other things on top, like dropping the test-sha1.sh script, having an unsafe pointer embedded within the_hash_algo, or something else entirely. But those can be done on top, or not at all, and I don't want to hold up this series for them.