Internal review

[iyourshaw]

• removed comments mentioning JER

[Michael7371]

@iyourshaw I am getting this error when trying to build the libraries and java bindings:

docker compose -f docker-compose-build.yml up --build -d

 => ERROR [build-shared 5/7] ADD ./run-lib.sh                                            /build                                                                                                                                               0.0s 
------
 > [build-shared 5/7] ADD ./run-lib.sh                                            /build:
------
Dockerfile-build:29
--------------------
  27 |     ADD ./asn1_codec/asn1c_combined/generated-files/2024.tar.gz /build
  28 |     ADD ./CMakeLists.txt                                        /build
  29 | >>> ADD ./run-lib.sh                                            /build
  30 |     COPY ./src                                                  /build/src/
  31 |
--------------------
failed to solve: failed to compute cache key: failed to calculate checksum of ref 0c9d7b2e-ad0f-48c6-9395-b528cb19f407::wvek12ufytlrxfypondu2ybp9: "/run-lib.sh": not found

I don't see a run-lb.sh file in the repo.

[iyourshaw]

@iyourshaw I am getting this error when trying to build the libraries and java bindings:

docker compose -f docker-compose-build.yml up --build -d

 => ERROR [build-shared 5/7] ADD ./run-lib.sh                                            /build                                                                                                                                               0.0s 
------
 > [build-shared 5/7] ADD ./run-lib.sh                                            /build:
------
Dockerfile-build:29
--------------------
  27 |     ADD ./asn1_codec/asn1c_combined/generated-files/2024.tar.gz /build
  28 |     ADD ./CMakeLists.txt                                        /build
  29 | >>> ADD ./run-lib.sh                                            /build
  30 |     COPY ./src                                                  /build/src/
  31 |
--------------------
failed to solve: failed to compute cache key: failed to calculate checksum of ref 0c9d7b2e-ad0f-48c6-9395-b528cb19f407::wvek12ufytlrxfypondu2ybp9: "/run-lib.sh": not found

I don't see a run-lb.sh file in the repo.

good catch, that is no longer used & was present on my machine but not in github, fixed it

[Michael7371]

@iyourshaw can you use a tag of the gradle/actions/setup-gradle instead of a commit sha?

[iyourshaw]

@iyourshaw can you use a tag of the gradle/actions/setup-gradle instead of a commit sha?

Yes, updated it to use the latest @v5 tag

[Michael7371]

@iyourshaw Can you provide the swagger docs in the API folder? Or add a /docs folder and put it in there? Spring should be able to auto generate it for you.

[Michael7371]

@iyourshaw j2735-2024-api\src\main\resources\application.yaml: Please document reasonings for the text-buffer-size and uper-buffer-size variable definitions.

[Michael7371]

@iyourshaw Fix extra new-line character between line 64 and 65 in the main README.md

[Michael7371]

@iyourshaw consider pulling out hard coded payloads in MessageFrameCodecTest into files with descriptive names, like how jpo-asn-pojos does it.

[Michael7371]

@iyourshaw it looks like the asnapplication.dll and libasnapplication.so files are included in the /test/resources folder of the j2735-2024-ffm-lib project. Can you include documentation on how that should be updated if the asn1_codec is changed? It looks like these files are required for the unit tests.
Maybe this can be documented in the /lib/README.md file?

[Michael7371]

@iyourshaw MessageFrameCodec.java:

1. [Blocking] Check if the incoming uper bytes is greater than the text buffer before converting: bytes.length > textBufferSize
2. [Blocking] Instead of using "xer" and "uper" string values consider using an enum object.
3. [Blocking] Consider catching potential exceptions from the SymbolLookup.libraryLookup call (line 87). Also consider checking if the input libraryPath file exists before trying to use it.
4. [Non-blocking]: error log on line 152 should be a log.error instead of a log.info

Also, I don't have the best understanding on memory allocation, but this is chatGPT's thoughts on your implementation. It might be completely wrong but figured I'd let you know:

Foreign Memory / Arena Lifetime Mismanagement
Issue A: loadLibrary() Arena Lifetime
private void loadLibrary(Path libraryPath) {
    Arena arena = Arena.ofAuto();
    SymbolLookup lookup = SymbolLookup.libraryLookup(libraryPath, arena);
    ...
}


Arena.ofAuto() creates an auto-closing arena, which is garbage-collected automatically.

When this method exits, the arena may be closed immediately.

The SymbolLookup object and symbols loaded from it (like convert_bytes) may become invalid, depending on the platform and JVM implementation.

Impact: You risk dangling symbol lookups. On some platforms, this will cause a Segmentation fault or IllegalStateException when calling native code.

✅ Fix: Use Arena.global() (persistent) or a static Arena field so that the symbol lookup remains valid for the JVM lifetime.

Example:

private static final Arena GLOBAL_ARENA = Arena.global();
private void loadLibrary(Path libraryPath) {
    SymbolLookup lookup = SymbolLookup.libraryLookup(libraryPath, GLOBAL_ARENA);
    ...
}

[Michael7371]

@iyourshaw convert.c review:

1. [non-blocking]: I think this exit(EXIT_FAILURE); code will cause the entire JVM to exit. Consider return -1 (or other error responses) and handle the logging more gracefully in the java code.
2. [blocking]: It seems like instead of truncating the num_encoded_bytes we should instead return an error (line 105).
3. [non-blocking]: Add null checking to the abbrev_to_syntax function:

static enum asn_transfer_syntax abbrev_to_syntax(const char * abbrev) {
    if (!abbrev) {
        fprintf(stderr, "Error: NULL encoding parameter\n");
        return -1;  // Return error code
    }
    
    if (strcmp("xer", abbrev) == 0) {
        return ATS_CANONICAL_XER;
    }
    if (strcmp("uper", abbrev) == 0) {
        return ATS_UNALIGNED_BASIC_PER;
    }
    fprintf(stderr, "Unknown encoding: %s  Expect 'xer' or 'uper'.\n", abbrev);
    return -1;  // Return error instead of exit()
}

convert.h review:

1. [blocking] typo on line 30 - J2375 should be J2735

[Michael7371]

Just a few comments to make the repo even more awesome! Nice work with all of this, it is really cool!

[Michael7371]

@linda-baker can you review the C code (in /src) for any potential memory leaks? I am not an expert in C and would think it would be good if you can take a closer look.

[linda-baker]

I didn't see any memory leak issues, the buffer handling seems solid.

The only thing that confuses me in the C code is the usage of "exit(EXIT_FAILURE);" when there's an error. I'm not familiar with that function, in my experience with C I'm used to returning -1 (or -2, -3 etc if you want to differentiate) to indicate errors to the caller. It looks like the way you do it works and you probably know something I don't here, but do you really want to terminate if you get an error, or would it be better to pass the error back up to MessageFrameCodec so you can course-correct from there?

[linda-baker]

I saw you pushed some changes the convert.c error handling, which resolves my only concern for the C parts of the project. Should you also change MessageFrameCodec.java to check for that -1 or err_buf to support that change?

[iyourshaw]

I didn't see any memory leak issues, the buffer handling seems solid.

The only thing that confuses me in the C code is the usage of "exit(EXIT_FAILURE);" when there's an error. I'm not familiar with that function, in my experience with C I'm used to returning -1 (or -2, -3 etc if you want to differentiate) to indicate errors to the caller. It looks like the way you do it works and you probably know something I don't here, but do you really want to terminate if you get an error, or would it be better to pass the error back up to MessageFrameCodec so you can course-correct from there?

The reason the code had "exit" statements was it was originally part of a standalone command line tool, but you are right, it's better to return an error code since it's now a library with no "main" method. I've updated the C and Java library code, and will also add some unit tests to the Java to test error cases.

[iyourshaw]

@iyourshaw MessageFrameCodec.java:

1. [Blocking] Check if the incoming uper bytes is greater than the text buffer before converting: bytes.length > textBufferSize
2. [Blocking] Instead of using "xer" and "uper" string values consider using an enum object.
3. [Blocking] Consider catching potential exceptions from the SymbolLookup.libraryLookup call (line 87). Also consider checking if the input libraryPath file exists before trying to use it.
4. [Non-blocking]: error log on line 152 should be a log.error instead of a log.info

1. Added byte array size checks
2. Added AsnEncoding enum
3. Added try..catch with logging
4. Fixed
5. For the issue with the lifetime of the symbol lookup, I kept it as-is, and added comment on why it is using a garbage collected arena instead of the global arena. Basically because the symbol lookup is assigned to a static variable, so it won't get garbage collected except under unusual circumstances, but keeping it that way to prevent a memory leak in case those unusual circumstances happen.

[iyourshaw]

@iyourshaw convert.c review:

1. [non-blocking]: I think this exit(EXIT_FAILURE); code will cause the entire JVM to exit. Consider return -1 (or other error responses) and handle the logging more gracefully in the java code.
2. [blocking]: It seems like instead of truncating the num_encoded_bytes we should instead return an error (line 105).
3. [non-blocking]: Add null checking to the abbrev_to_syntax function:

1. I don't know if it would exit the JVM but agree it wasn't graceful, fixed it to return an error code and message per Linda's comment.
2. Fixed
3. Fixed

convert.h review:

1. [blocking] typo on line 30 - J2375 should be J2735

1. Fixed

[Michael7371]

Looks good, thanks for addressing my comments!