fabiocfabini commented on PR #3732:
URL: https://github.com/apache/avro/pull/3732#issuecomment-4425627920
> #3732 and #3635 😆 I want to merge one before the release, which one should
we use? @Kwen @fabiocfabini I trust you to let me know objectively!
Right. I can see 2 issues with #3635:
1. `read_bytes()` does not compile. It uses undeclared variable `str_len`
(likely a remnant from `read_string()`. Merely a bug that can be fixed by
replacing `str_len` with `len`:
```diff
diff --git a/lang/c/src/encoding_binary.c b/lang/c/src/encoding_binary.c
index 8d7cd0e3d..ba9b08a36 100644
--- a/lang/c/src/encoding_binary.c
+++ b/lang/c/src/encoding_binary.c
@@ -134,9 +134,9 @@ static int read_bytes(avro_reader_t reader, char
**bytes, int64_t * len)
}
max_available = avro_max_read(reader);
- if (max_available >= 0 && str_len > max_available) {
+ if (max_available >= 0 && len > max_available) {
avro_set_error("String length %" PRId64 " is greater than
available buffer size %" PRId64,
- str_len, max_available);
+ len, max_available);
return ERANGE;
}
```
2. As already questioned in a thread on the PR, the introduction of function
`avro_max_read()`, whose purpose is to detect the maximum read avro can make,
does not have the same semantic behavior depending on if the reader is a
`_avro_reader_file_t` or `_avro_reader_memory_t` and consequently causes
several tests to fail. Namely:
```console
The following tests FAILED:
1 - quickstop (Failed)
6 - test_avro_commons_schema (Failed)
7 - memcheck_test_avro_commons_schema (Failed)
12 - test_avro_values (Failed)
13 - memcheck_test_avro_values (Failed)
24 - test_avro_1087 (Failed)
25 - memcheck_test_avro_1087 (Failed)
30 - test_avro_1237 (Failed)
31 - memcheck_test_avro_1237 (Failed)
32 - test_avro_1238 (Failed)
33 - memcheck_test_avro_1238 (Failed)
34 - test_avro_1279 (Failed)
35 - memcheck_test_avro_1279 (Failed)
36 - test_avro_1405 (Failed)
37 - memcheck_test_avro_1405 (Failed)
38 - test_avro_1572 (Failed)
39 - memcheck_test_avro_1572 (Failed)
40 - test_avro_data (Failed)
43 - test_avro_1379 (Failed)
44 - memcheck_test_avro_1379 (Failed)
45 - test_avro_1691 (Failed)
46 - memcheck_test_avro_1691 (Failed)
47 - test_avro_1906 (Failed)
48 - memcheck_test_avro_1906 (Failed)
49 - test_avro_1904 (Failed)
50 - memcheck_test_avro_1904 (Failed)
```
The PRs are essentially the same other then the addition of additional test
case 4246 I introduced. That being said I think this one should be merged
instead. @Kwen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
