diff mbox series

[3/6] fixup! reftable: rest of library

Message ID 58f2b0394546e8da2922adcbc38bdb6b53f2b313.1606545878.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Minimal patches to let reftable pass the CI builds | expand

Commit Message

Johannes Schindelin Nov. 28, 2020, 6:44 a.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

0-sized arrays are actually not portable.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 reftable/stack_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jeff King Dec. 1, 2020, 10:26 a.m. UTC | #1 
On Sat, Nov 28, 2020 at 06:44:35AM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 0-sized arrays are actually not portable.

Definitely.

>  static void test_sizes_to_segments_empty(void)
>  {
> -	uint64_t sizes[0];
> +	uint64_t sizes[1];
>  
>  	int seglen = 0;
>  	struct segment *segs =
> -		sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));
> +		sizes_to_segments(&seglen, sizes, 0);
>  	EXPECT(seglen == 0);
>  	reftable_free(segs);

I think passing:

  sizes_to_segments(&seglen, NULL, 0);

may make the code more obvious. Unlike system functions like memcpy(),
we can be assured of whether our functions avoid looking at a
zero-length array (and size_to_segments does follow that rule).

  This function, of course, is nonsense that real code would not do, and
  is just unit-testing sizes_to_segments. I'm not wild in general about
  having a parallel suite of C tests that does not interact with our
  usual tests, but it may be the least bad way to benefit from the
  unit-test coverage that reftable ships with. As a rule, I'd much
  rather see a tool exposing functionality to the command-line, which
  can then be driven independently. I recognize that can end up
  complicated, though.

-Peff
Han-Wen Nienhuys Dec. 1, 2020, 11:10 a.m. UTC | #2 
On Tue, Dec 1, 2020 at 11:26 AM Jeff King <peff@peff.net> wrote:
>
> On Sat, Nov 28, 2020 at 06:44:35AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 0-sized arrays are actually not portable.
>
> Definitely.
>
> >  static void test_sizes_to_segments_empty(void)
> >  {
> > -     uint64_t sizes[0];
> > +     uint64_t sizes[1];
> >
> >       int seglen = 0;
> >       struct segment *segs =
> > -             sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));
> > +             sizes_to_segments(&seglen, sizes, 0);
> >       EXPECT(seglen == 0);
> >       reftable_free(segs);
>
> I think passing:
>
>   sizes_to_segments(&seglen, NULL, 0);
>
> may make the code more obvious. Unlike system functions like memcpy(),
> we can be assured of whether our functions avoid looking at a
> zero-length array (and size_to_segments does follow that rule).
>
>   This function, of course, is nonsense that real code would not do, and

This test was added because 'real' code did this. In particular, if
you initialize a stack of reftables, the stack has zero elements. The
test was for the following bugfix

https://github.com/google/reftable/commit/b2e42ecb54e413e494c1fcc13c21e24422645007

Changing the array size to 1 here also prevents Valgrind to mark a
regression as a OOB write.
Jeff King Dec. 1, 2020, 11:57 a.m. UTC | #3 
On Tue, Dec 01, 2020 at 12:10:14PM +0100, Han-Wen Nienhuys wrote:

> > I think passing:
> >
> >   sizes_to_segments(&seglen, NULL, 0);
> >
> > may make the code more obvious. Unlike system functions like memcpy(),
> > we can be assured of whether our functions avoid looking at a
> > zero-length array (and size_to_segments does follow that rule).
> >
> >   This function, of course, is nonsense that real code would not do, and
> 
> This test was added because 'real' code did this. In particular, if
> you initialize a stack of reftables, the stack has zero elements. The
> test was for the following bugfix
> 
> https://github.com/google/reftable/commit/b2e42ecb54e413e494c1fcc13c21e24422645007
> 
> Changing the array size to 1 here also prevents Valgrind to mark a
> regression as a OOB write.

Sorry to be unclear. I meant that real code would not allocate a static
zero-length array (though it might do so on the heap). I agree that
making sure the function behaves in the 0-segment case is quite
reasonable.

Passing NULL actually makes the test more rigorous IMHO (because it is a
strict superset of failure modes, and is something an actual caller
might do if it were dynamically growing an array that started at NULL).

-Peff
diff mbox series

Patch

diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index c35abd7301..cf2e32a25c 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -579,11 +579,11 @@  static void test_sizes_to_segments(void)
 
 static void test_sizes_to_segments_empty(void)
 {
-	uint64_t sizes[0];
+	uint64_t sizes[1];
 
 	int seglen = 0;
 	struct segment *segs =
-		sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));
+		sizes_to_segments(&seglen, sizes, 0);
 	EXPECT(seglen == 0);
 	reftable_free(segs);
 }