Allow changed reads #286

krnowak · 2022-02-26T21:43:02Z

Hi,

I'd like to show you some PoC code that allows reading contents of the changed files in the archive (that is - doing zip_fopen without the ZIP_FL_UNCHANGED flag). This is implemented as an opt-in feature of zip_source - it needs to say that it supports the ZIP_SOURCE_ALLOW_CHANGED_READ command, and that command should return 1 if it's actually allowed. In such case, zip_fread will use the source to read data from it, instead of returning ZIP_ER_CHANGED. The feature should be backwards compatible, as no builtin non-layered sources support this command, and the builtin layered sources (encryption, window and crc) are deferring to the underlying source.

My usecase was basically to have the zip archive to serve as a backend (next to the "directory and files" backend) for a stream source, which can provide input or output streams. This change would allow me to open an output stream, write some data to it, and then close it; later I could open the same stream, but as an input and read from it.

Would such a feature be considered for inclusion to the project?

dillof · 2022-03-04T07:40:25Z

Thank you. The functionality sounds good, but the implementation will need some refinement.

We'll discuss it internally and get back to you, probably some time next week.

krnowak · 2022-03-04T08:52:23Z

Thank you. The functionality sounds good, but the implementation will need some refinement.

Cool, good to know, thanks. About implementation - surely it will need refinement. Just for the record - I tried extending _zip_source_zip_new but it was rather a frustrating exercise, as changing/adapting something in this function always seemed to break that or another test case. That's why I went with the duplication of code - I assumed it should be just fine for the PoC code.

We'll discuss it internally and get back to you, probably some time next week.

Alright, I'll fix the errors from CI in the meantime.

dillof · 2022-03-10T10:31:51Z

The feature you add to sources has broader application than reading changed data, so a more general name would better describe it. We've come up with ZIP_SOURCE_SUPPORTS_REOPEN.

Also, by moving support for it into the infrastructure, we can minimise diffs for each source:

index f6db2a71..d59ab828 100644
--- a/lib/zip_source_layered.c
+++ b/lib/zip_source_layered.c
@@ -62,6 +62,9 @@ zip_source_layered_create(zip_source_t *src, zip_source_layered_callback cb, voi
     if (zs->supports < 0) {
         zs->supports = ZIP_SOURCE_SUPPORTS_READABLE;
     }
+    else if ((zip_source_supports(src) & ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN)) == 0) {
+        zs->supports &= ~ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN);
+    }
 
     return zs;
 }
diff --git a/lib/zip_source_supports.c b/lib/zip_source_supports.c
index 66a5303c..8ccc41bc 100644
--- a/lib/zip_source_supports.c
+++ b/lib/zip_source_supports.c
@@ -63,3 +63,8 @@ zip_source_make_command_bitmap(zip_source_cmd_t cmd0, ...) {
 
     return bitmap;
 }
+
+
+bool zip_supports_multiple_open(zip_source_t *src) {
+    return (zip_source_supports(src) & ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN)) != 0;
+}

Also, we don't need to make another call that only ever returns 1. If a source says it supports reopen, then no further checks are needed.

With that, all that's needed in each source is to add ZIP_SOURCE_SUPPORTS_REOPEN to the bitmap of supported commands in ZIP_SOURCE_SUPPORTS.

Please do not duplicate the main part of zip_source_zip_new. Make a function you can call with a zip_source_t and a zip_stat_t describing the format of the existing data source and flags that describe the desired data format; this function should figure out which layers need to be put on top to make that happen (this is the part you duplicated); then call that function from both cases.

Also, please don't add a special case to fread.c. For most tests we use ziptool_regress, which should support all of the functions you need. (The cat sub-command already reads changed data.) Please use that in your tests.

On a stylistic note, please remove spaces before opening parentheses in function calls (function(arguments) rather than function (arguements)).

krnowak · 2022-03-11T07:29:36Z

The feature you add to sources has broader application than reading changed data, so a more general name would better describe it. We've come up with ZIP_SOURCE_SUPPORTS_REOPEN.

This is certainly a better name. Will use it, thanks.

Also, by moving support for it into the infrastructure, we can minimise diffs for each source:

index f6db2a71..d59ab828 100644
--- a/lib/zip_source_layered.c
+++ b/lib/zip_source_layered.c
@@ -62,6 +62,9 @@ zip_source_layered_create(zip_source_t *src, zip_source_layered_callback cb, voi
     if (zs->supports < 0) {
         zs->supports = ZIP_SOURCE_SUPPORTS_READABLE;
     }
+    else if ((zip_source_supports(src) & ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN)) == 0) {
+        zs->supports &= ~ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN);
+    }
 
     return zs;
 }
diff --git a/lib/zip_source_supports.c b/lib/zip_source_supports.c
index 66a5303c..8ccc41bc 100644
--- a/lib/zip_source_supports.c
+++ b/lib/zip_source_supports.c
@@ -63,3 +63,8 @@ zip_source_make_command_bitmap(zip_source_cmd_t cmd0, ...) {
 
     return bitmap;
 }
+
+
+bool zip_supports_multiple_open(zip_source_t *src) {
+    return (zip_source_supports(src) & ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN)) != 0;
+}

Also, we don't need to make another call that only ever returns 1. If a source says it supports reopen, then no further checks are needed.

With that, all that's needed in each source is to add ZIP_SOURCE_SUPPORTS_REOPEN to the bitmap of supported commands in ZIP_SOURCE_SUPPORTS.

This is a nice simplification, thanks. The only thing I thought is that the source could decide during ZIP_SOURCE_SUPPORTS_REOPEN command execution whether the reopening is possible at this moment (because we are still appending to the internal buffer or something), but I suppose I can return an error from ZIP_SOURCE_OPEN or ZIP_SOURCE_STAT.

Please do not duplicate the main part of zip_source_zip_new. Make a function you can call with a zip_source_t and a zip_stat_t describing the format of the existing data source and flags that describe the desired data format; this function should figure out which layers need to be put on top to make that happen (this is the part you duplicated); then call that function from both cases.

Also, please don't add a special case to fread.c. For most tests we use ziptool_regress, which should support all of the functions you need. (The cat sub-command already reads changed data.) Please use that in your tests.

Will try it out.

On a stylistic note, please remove spaces before opening parentheses in function calls (function(arguments) rather than function (arguements)).

Oops, sorry about that. My personal style got in. Will fix it.

dillof · 2022-03-11T08:21:12Z

Also, we don't need to make another call that only ever returns 1. If a source says it supports reopen, then no further checks are needed.
With that, all that's needed in each source is to add ZIP_SOURCE_SUPPORTS_REOPEN to the bitmap of supported commands in ZIP_SOURCE_SUPPORTS.

This is a nice simplification, thanks. The only thing I thought is that the source could decide during ZIP_SOURCE_SUPPORTS_REOPEN command execution whether the reopening is possible at this moment (because we are still appending to the internal buffer or something), but I suppose I can return an error from ZIP_SOURCE_OPEN or ZIP_SOURCE_STAT.

We need a promise from the source that it will allow a second open, otherwise zip_close() will fail. So the source has to decide up front if it can support that, otherwise it should simply say it doesn't. Having zip_close() succeed is much more important than allowing reading changed data.

krnowak · 2022-03-21T11:16:26Z

Also, by moving support for it into the infrastructure, we can minimise diffs for each source:

index f6db2a71..d59ab828 100644
--- a/lib/zip_source_layered.c
+++ b/lib/zip_source_layered.c
@@ -62,6 +62,9 @@ zip_source_layered_create(zip_source_t *src, zip_source_layered_callback cb, voi
     if (zs->supports < 0) {
         zs->supports = ZIP_SOURCE_SUPPORTS_READABLE;
     }
+    else if ((zip_source_supports(src) & ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN)) == 0) {
+        zs->supports &= ~ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN);
+    }
 
     return zs;
 }

I also added the case if (source supports reopen) zs->supports |= SUPPORTS_REOPEN here, so the layered sources support reopening if the underlying source also supports it.

Please do not duplicate the main part of zip_source_zip_new. Make a function you can call with a zip_source_t and a zip_stat_t describing the format of the existing data source and flags that describe the desired data format; this function should figure out which layers need to be put on top to make that happen (this is the part you duplicated); then call that function from both cases.

In the end I extended the zip_source_zip_new function, because the new function would basically be taking all the parameters of the zip_source_zip_new to determine the layers (srcza, srcidx, start, len would be needed for windowed layer, start and len for determining the partial read, password for decryption layer, so meh). But the additions were not that messy in the end (I think).

Also, please don't add a special case to fread.c. For most tests we use ziptool_regress, which should support all of the functions you need. (The cat sub-command already reads changed data.) Please use that in your tests.

I replaced that with ziptool, but needed to add the special "{add,replace}_reopenable" command to ziptool-regress, as I haven't added support for reopening to any of the builtin sources yet. I'm not sure how to do it yet (either add ZIP_SOURCE_SUPPORTS_REOPEN to the supports set, or add zip_file_{add,replace}_reopenable (and similar for buffer source) or add ZIP_FL_REOPENABLE to zip_flags_t), so I decided to leave it out and add a simple reopenable read-only buffer source to ziptool regress.

TODO:

update docs
add a test that adds a reopenable file to a zip archive, and then adds a part of the reopenable file to another zip archive

lib/zip_source_layered.c

lib/zip_source_zip_new.c

dillof · 2022-03-21T12:04:08Z

Also, by moving support for it into the infrastructure, we can minimise diffs for each source:
index f6db2a71..d59ab828 100644
--- a/lib/zip_source_layered.c
+++ b/lib/zip_source_layered.c
@@ -62,6 +62,9 @@ zip_source_layered_create(zip_source_t *src, zip_source_layered_callback cb, voi
     if (zs->supports < 0) {
         zs->supports = ZIP_SOURCE_SUPPORTS_READABLE;
     }
+    else if ((zip_source_supports(src) & ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN)) == 0) {
+        zs->supports &= ~ZIP_SOURCE_MAKE_COMMAND_BITMASK(ZIP_SOURCE_SUPPORTS_REOPEN);
+    }
 
     return zs;
 }
I also added the case if (source supports reopen) zs->supports |= SUPPORTS_REOPEN here, so the layered sources support reopening if the underlying source also supports it.

That's not a safe assumption, see my review comment.

Please do not duplicate the main part of zip_source_zip_new. Make a function you can call with a zip_source_t and a zip_stat_t describing the format of the existing data source and flags that describe the desired data format; this function should figure out which layers need to be put on top to make that happen (this is the part you duplicated); then call that function from both cases.

In the end I extended the zip_source_zip_new function, because the new function would basically be taking all the parameters of the zip_source_zip_new to determine the layers (srcza, srcidx, start, len would be needed for windowed layer, start and len for determining the partial read, password for decryption layer, so meh). But the additions were not that messy in the end (I think).

Yes, that approach works fine.

Also, please don't add a special case to fread.c. For most tests we use ziptool_regress, which should support all of the functions you need. (The cat sub-command already reads changed data.) Please use that in your tests.

I replaced that with ziptool, but needed to add the special "{add,replace}_reopenable" command to ziptool-regress, as I haven't added support for reopening to any of the builtin sources yet. I'm not sure how to do it yet (either add ZIP_SOURCE_SUPPORTS_REOPEN to the supports set, or add zip_file_{add,replace}_reopenable (and similar for buffer source) or add ZIP_FL_REOPENABLE to zip_flags_t), so I decided to leave it out and add a simple reopenable read-only buffer source to ziptool regress.

Just advertise support for reopening to sources that can handle it. That should be most of the built in sources.

TODO:

update docs

add a test that adds a reopenable file to a zip archive, and then adds a part of the reopenable file to another zip archive

That's a more complicated test than necessary. For now, just add a file and read it with zip_tool cat.

krnowak · 2022-03-23T06:14:02Z

I replaced that with ziptool, but needed to add the special "{add,replace}_reopenable" command to ziptool-regress, as I haven't added support for reopening to any of the builtin sources yet. I'm not sure how to do it yet (either add ZIP_SOURCE_SUPPORTS_REOPEN to the supports set, or add zip_file_{add,replace}_reopenable (and similar for buffer source) or add ZIP_FL_REOPENABLE to zip_flags_t), so I decided to leave it out and add a simple reopenable read-only buffer source to ziptool regress.

Just advertise support for reopening to sources that can handle it. That should be most of the built in sources.

Ok, will do - at first I didn't want to change the built in sources (I dunno, someone relies on getting ZIP_ER_CHANGED from zip_fopen after adding a buffer source there?), but will update the support bitmasks.

TODO:

update docs

add a test that adds a reopenable file to a zip archive, and then adds a part of the reopenable file to another zip archive

That's a more complicated test than necessary. For now, just add a file and read it with zip_tool cat.

Yeah, that's already done I'd say. With this TODO item, I wanted to test the code path where we do a partial read of the changed data and this seems to be the only way.

krnowak · 2022-03-23T06:40:16Z

add a test that adds a reopenable file to a zip archive, and then adds a part of the reopenable file to another zip archive

That's a more complicated test than necessary. For now, just add a file and read it with zip_tool cat.

Yeah, that's already done I'd say. With this TODO item, I wanted to test the code path where we do a partial read of the changed data and this seems to be the only way.

Just realized that I could do it with just one archive.

dillof · 2022-03-23T09:36:43Z

I replaced that with ziptool, but needed to add the special "{add,replace}_reopenable" command to ziptool-regress, as I haven't added support for reopening to any of the builtin sources yet. I'm not sure how to do it yet (either add ZIP_SOURCE_SUPPORTS_REOPEN to the supports set, or add zip_file_{add,replace}_reopenable (and similar for buffer source) or add ZIP_FL_REOPENABLE to zip_flags_t), so I decided to leave it out and add a simple reopenable read-only buffer source to ziptool regress.

Just advertise support for reopening to sources that can handle it. That should be most of the built in sources.

Ok, will do - at first I didn't want to change the built in sources (I dunno, someone relies on getting ZIP_ER_CHANGED from zip_fopen after adding a buffer source there?), but will update the support bitmasks.

TODO:

update docs

add a test that adds a reopenable file to a zip archive, and then adds a part of the reopenable file to another zip archive

That's a more complicated test than necessary. For now, just add a file and read it with zip_tool cat.

Yeah, that's already done I'd say. With this TODO item, I wanted to test the code path where we do a partial read of the changed data and this seems to be the only way.

To test partial reads, we should add a command to zip_tool that outputs part of a file, like cat_partial <index> <start> <length>. We try to avoid writing programs for individual test cases.

krnowak · 2022-03-25T21:11:36Z

Updated, still need to write some docs.

add a test that adds a reopenable file to a zip archive, and then adds a part of the reopenable file to another zip archive

That's a more complicated test than necessary. For now, just add a file and read it with zip_tool cat.

Yeah, that's already done I'd say. With this TODO item, I wanted to test the code path where we do a partial read of the changed data and this seems to be the only way.

To test partial reads, we should add a command to zip_tool that outputs part of a file, like cat_partial <index> <start> <length>. We try to avoid writing programs for individual test cases.

So my idea was to have more or less something like this:

./ziptool testbuffer.zip \
    cat 0 \
    replace_file_contents 0 "Overwritten\n" \
    cat 0 \
    add_from_zip part.txt testbuffer.zip 0 4 5 \
    cat 1

But that will open testbuffer.zip again, where the modification from replace_file_contents does not exist. Meh.

cat_partial <index> <start> <length> currently can be implemented in term of fopen, fread to throwaway buffer, then fread + fwrite to stdout, then fclose. Which is not ideal, because fopen opens the source with start and len set to zeroes, and I wanted to test the partial reads paths.

krnowak · 2022-03-26T07:20:06Z

cat_partial <index> <start> <length> currently can be implemented in term of fopen, fread to throwaway buffer, then fread + fwrite to stdout, then fclose. Which is not ideal, because fopen opens the source with start and len set to zeroes, and I wanted to test the partial reads paths.

Or by zip_source_zip and using zip_source_{open,read,close,free}, maybe with some seeking. I'll have a look. Didn't notice before that zip_source_* is actually a part of API.

krnowak · 2022-03-26T19:06:07Z

Implemented cat_partial in ziptool and added two more tests.

krnowak · 2022-03-27T20:01:07Z

For now I can say that I have no idea why the tests fail on appveyor. At first I thought it's because of enabling reopen in zip_source_file_common.c, but removing the support didn't change a thing. I only have a feeling that in appveyor I'm getting no stderr output from the failed tests.

I also did a small stab at documentation, but I'll admit that the mdoc format is pretty much alien to me.

krnowak · 2022-04-03T07:36:27Z

Updated, the issue with windows failures and hopefully with the fuzzer is resolved.

dillof · 2022-04-13T21:06:27Z

Sorry, I've been busy lately, but I'll have a look at it next week.

Thank you for your continued work on this issue.

krnowak · 2022-04-18T10:49:09Z

lib/zip_source_zip_new.c

+        src = srcza->entry[srcidx].source;
+        zip_source_keep(src);


Used the branch a bit more and there was one issue - having two zip_file_t to the same reopened source messes up reads - reading from one zip_file_t affects reading from the another. It normally works for unchanged sources, because window source does seeks before calling read on the underlying source. I think here we will need to wrap the reopened source into window source here too.

We should probably address this in zip_source_read: keep track of how many opens we have, and use seek if there are more than one. And disallow a second open if the source doesn't support seeking.

This is an existing problem, so we can address this after the merge.

src/ziptool.c

dillof · 2022-05-18T09:22:19Z

lib/zip_fclose.c


-    if (zf->src)
+    if (zf->src) {
+        zip_source_close(zf->src);


zip_source_free does a close, so this call is unnecessary.

Ok, I can revert this change. I'll only point you to the commit message in 4a97e13. Now this probably won't be a problem any more, because we always wrap the bottom-most source in some other sources, which happen to have ref count of 1, so they'll be closed on free.

I'd still say that zip_source_free is a misnomer (should be rather something like zip_source_unref), because it's freeing things only if ref count drops to 0. So if refcount is still higher, no free will happen and no closing either.

You're right, we need to keep track which references called open the source. We will split the source struct in two parts, the actual source and a reference. But we can do this after the merge.

dillof · 2022-05-18T09:23:13Z

man/zip_source_function.mdoc

+.Dv ZIP_SOURCE_SUPPORTS_REOPEN
+will allow calling
+.Fn zip_fopen
+without the ZIP_FL_UNCHANGED flag to read the modified data.


At the zip_source level, it means that you can read the data multiple times. If you want to document what it means for zip_fopen, document it there.

Sorry, I don't understand this. You could read the data multiple times without this PR - it was possible because window source takes care of repositioning the underlying source by seeking it to the desired offset before doing a read. The catch was that it was only possible for unmodified data, and since unmodified data is backed by a source that implements seeking, reading data multiple times always worked.

regress/fopen_multiple.c

lib/zip_source_zip_new.c

Currently none of the sources provided by libzip supports reopening, but it is possible to write our own that does by using zip_source_function.

The only source that is not supporting reopening is the file-backed source. It works on Linux, but on some Windows platforms there are failures I can't debug.

We won't get the ZIP_ER_CHANGED errors, because built in sources support reopening.

When cat in ziptool got implemented in terms of sources instead of files, reading encrypted data broke. Not using the default password stored in the archive was the culprit.

We will implement a test for intertwined reads from multiple opened files backed by the same source in terms of those functions.

krnowak · 2022-05-24T18:47:02Z

Updated. The only review issue left to address is the one about documentation, but I had some doubts about that as commented above. Other unresolved issues are solved, but had a comment or question about it anyway.

Implementing cat in terms of sources instead of files uncovered an issue with reading encrypted files (decrypt-correct-password-pkware-2.test was failing), hopefully the fix I have made is correct.

dillof · 2022-05-18T10:02:17Z

lib/zip_source_zip_new.c

+        src = srcza->entry[srcidx].source;
+        zip_source_keep(src);


We should probably address this in zip_source_read: keep track of how many opens we have, and use seek if there are more than one. And disallow a second open if the source doesn't support seeking.

This is an existing problem, so we can address this after the merge.

dillof · 2022-06-28T09:15:32Z

lib/zip_fclose.c


-    if (zf->src)
+    if (zf->src) {
+        zip_source_close(zf->src);


You're right, we need to keep track which references called open the source. We will split the source struct in two parts, the actual source and a reference. But we can do this after the merge.

dillof · 2022-06-28T09:20:23Z

lib/zip_source_zip.c

+        password = NULL;
+    }
+
+    return _zip_source_zip_new(srcza, srcidx, flags, start, (zip_uint64_t)len, password, error);


zip_set_default_password already turns an empty password into NULL, so the check here is redundant. Just pass srcza->default_password to _zip_source_zip_new.

dillof · 2022-06-28T09:38:48Z

Sorry for letting this sit for so long. We finally merged it.

You did excellent work, thank you.

…10.0 In libzip versions prior to 1.10.0, you couldn't do a read for a changed file: this would result in an error. This is tested in our .phpt test. Starting from version 1.10.0 [1] this is no longer an error. Therefore, we amend the test to check the behaviour depending on which version is used. [1] nih-at/libzip#286

krnowak force-pushed the allow-changed-reads branch from e734bd5 to c0e85bc Compare February 27, 2022 21:10

krnowak force-pushed the allow-changed-reads branch from d7f0cdd to 0409796 Compare March 21, 2022 11:00

dillof requested changes Mar 21, 2022

View reviewed changes

krnowak force-pushed the allow-changed-reads branch from 0409796 to c97e6d1 Compare March 25, 2022 20:53

krnowak force-pushed the allow-changed-reads branch from c97e6d1 to eda1834 Compare March 26, 2022 19:05

krnowak force-pushed the allow-changed-reads branch 3 times, most recently from a978b74 to 773e4aa Compare March 27, 2022 18:58

krnowak force-pushed the allow-changed-reads branch from 773e4aa to 7cf79d8 Compare April 2, 2022 19:10

krnowak requested a review from dillof April 13, 2022 20:59

krnowak commented Apr 18, 2022

View reviewed changes

krnowak force-pushed the allow-changed-reads branch from 9072958 to 4e78011 Compare April 18, 2022 12:34

dillof requested changes May 18, 2022

View reviewed changes

Add support for reopening

4cc039a

Currently none of the sources provided by libzip supports reopening, but it is possible to write our own that does by using zip_source_function.

krnowak added 9 commits May 24, 2022 20:25

Test reopening

389a5f9

Add support for reopen for built in sources

f5acde1

The only source that is not supporting reopening is the file-backed source. It works on Linux, but on some Windows platforms there are failures I can't debug.

Update fread test

699194a

We won't get the ZIP_ER_CHANGED errors, because built in sources support reopening.

Use a default password in zip source

0b876e4

When cat in ziptool got implemented in terms of sources instead of files, reading encrypted data broke. Not using the default password stored in the archive was the culprit.

Implement partial cat in ziptool

9011a65

Test partial reads in reopened files

168a960

Add opening and reading files in ziptool_regress

2b0fc64

We will implement a test for intertwined reads from multiple opened files backed by the same source in terms of those functions.

Add a test for reading from multiple files

a68477e

Update documentation

b13daae

krnowak force-pushed the allow-changed-reads branch from 4e78011 to b13daae Compare May 24, 2022 18:40

krnowak requested a review from dillof June 14, 2022 18:35

dillof approved these changes Jun 28, 2022

View reviewed changes

dillof merged commit a8fddb6 into nih-at:main Jun 28, 2022

krnowak deleted the allow-changed-reads branch December 21, 2022 11:33

nielsdos mentioned this pull request Jun 24, 2023

Fix GH-11520: Test fails due to compatibility issue with libzip 1.10.0 php/php-src#11525

Closed

Uh oh!

Allow changed reads #286

Allow changed reads #286

Uh oh!

Conversation

krnowak commented Feb 26, 2022

Uh oh!

dillof commented Mar 4, 2022

Uh oh!

krnowak commented Mar 4, 2022

Uh oh!

dillof commented Mar 10, 2022

Uh oh!

krnowak commented Mar 11, 2022

Uh oh!

dillof commented Mar 11, 2022

Uh oh!

krnowak commented Mar 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dillof commented Mar 21, 2022

Uh oh!

krnowak commented Mar 23, 2022

Uh oh!

krnowak commented Mar 23, 2022

Uh oh!

dillof commented Mar 23, 2022

Uh oh!

krnowak commented Mar 25, 2022

Uh oh!

krnowak commented Mar 26, 2022

Uh oh!

krnowak commented Mar 26, 2022

Uh oh!

krnowak commented Mar 27, 2022

Uh oh!

krnowak commented Apr 3, 2022

Uh oh!

dillof commented Apr 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krnowak commented May 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dillof commented Jun 28, 2022

Uh oh!

Reviewers