Provide a hook to setup iconv path at runtime

[malaterre]

No description provided.

[malaterre]

@eichelberg This is mostly a WIP but basically I need this API to specify where to search for esbd folders. Let me know if you want more details about the use case. Thanks !

[michaelonken]

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

It appears that you are using strcpy or strncpy functions that does not affirm the size of the destination array and does not automatically NULL-terminate strings leading to buffer overflows risks. We recommend you to apply bound checking or null terminator. Learn more - https://www.geeksforgeeks.org/why-strcpy-and-strncpy-are-not-safe-to-use/

[michaelonken]

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

We observed that your code contains out-of-bounds read which is a type of memory access error that occurs when a program reads data from a memory address outside of the bounds of a buffer. This can result in the program reading data that does not belong to it, which can cause crashes, incorrect behaviour, or even security vulnerabilities. To prevent this you can perform bounds checking before accessing an array or other data structure. Learn more - https://www.martellosecurity.com/kb/mitre/cwe/125/

[michaelonken]

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

We observed your code contains an out-of-bounds write which may lead to security issues such as crashing, information leaks, or even arbitrary code execution. You should perform bounds checking, validate input sizes and indices to ensure they stay within the allocated memory bounds. Learn more - https://github.com/harsh-bothra/SecurityExplained/blob/main/resources/cwe-787.md

[eichelberg]

@eichelberg This is mostly a WIP but basically I need this API to specify where to search for esbd folders. Let me know if you want more details about the use case. Thanks !

Is there a reason why you cannot use the DCMICONVPATH environment variable to point to the desired directory?

[malaterre]

@eichelberg This is mostly a WIP but basically I need this API to specify where to search for esbd folders. Let me know if you want more details about the use case. Thanks !

Is there a reason why you cannot use the DCMICONVPATH environment variable to point to the desired directory?

I am actually glad you raised this point this will a twofold answer:

1. Currently oficonv rely on getenv to access DCMICONVPATH. This may not work on windows depending on which c library you use. I believe this is what's documented as:

getenv operates only on the data structures accessible to the run-time library and not on the environment "segment" created for the process by the operating system.

• https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-wgetenv?view=msvc-170#remarks

So one way to work around this is to use GetEnvironmentVariable/SetEnvironmentVariable on windows system.

2. Assuming you have a setup where the current DCMICONVPATH mechanism is working. You still have an issue when more than one path is used on a single system (setenv/getenv is fundamentally non-thread safe). In our application we ship the dicom.dic as well as the esdb folders within the nuget package (=zip file). All the resources files gets extracted at runtime and eventually the C# application would call setenv(DCMICONVPATH), but this means another application could also run at the same time and change this value and we have a conflict if versions are different. I suspect there is the same sort of issues with python packaging (they each live in their own space).

[malaterre]

@eichelberg In case you are also wondering I am trying to go away from the current solution which makes use of

set(DCMTK_ENABLE_BUILTIN_OFICONV_DATA ON)
set(DCMTK_DEFAULT_DICT "builtin")

The above is working for us but is not convenient.

[eichelberg]

getenv operates only on the data structures accessible to the run-time library and not on the environment "segment" created for the process by the operating system.

That means that you cannot call SetEnvironmentVariable() to change an environment variable in the running process and then expect the next call to getenv() to report that new value.

You still have an issue when more than one path is used on a single system (setenv/getenv is fundamentally non-thread safe).

It is thread unsafe, but I am fairly certain that each process has its own set of environment variables, on Windows and on Posix systems. Another application calling setenv() cannot interfere with your application.

Anyway, I am not against adding a function in the oficonv module that allows you to set the iconv path manually.
Once the patch is complete and does not do stupid things like assuming that a path will never be longer than 4096 bytes, please let me know and I will happy to merge this into DCMTK.

[eichelberg]

One more thing: DcmSpecificCharacterSet::setIconvPath() should return an OFCondition, and should actually return an error when DCMTK has been compiled with a different conversion library (GNU libiconv or libc iconv) that does not support such API.

[jriesmeier]

By the way, I don't see a good reason why OFCharacterEncoding and DcmSpecificCharacterSet should be "polluted" with a static function that has nothing to do with these classes.

[malaterre]

By the way, I don't see a good reason why OFCharacterEncoding and DcmSpecificCharacterSet should be "polluted" with a static function that has nothing to do with these classes.

ACK. I was debating if this call could be generic for the other alternatives implementations. I'll make it oficonv specific. Thx

[malaterre]

getenv operates only on the data structures accessible to the run-time library and not on the environment "segment" created for the process by the operating system.

That means that you cannot call SetEnvironmentVariable() to change an environment variable in the running process and then expect the next call to getenv() to report that new value.

Uh. This is exactly what is working for me while _putenv/getenv combo failed for me.

You still have an issue when more than one path is used on a single system (setenv/getenv is fundamentally non-thread safe).

It is thread unsafe, but I am fairly certain that each process has its own set of environment variables, on Windows and on Posix systems. Another application calling setenv() cannot interfere with your application.

I stand corrected on this. Thanks

Anyway, I am not against adding a function in the oficonv module that allows you to set the iconv path manually. Once the patch is complete and does not do stupid things like assuming that a path will never be longer than 4096 bytes, please let me know and I will happy to merge this into DCMTK.

Ack

[malaterre]

@eichelberg / @jriesmeier How about this one ?

[eichelberg]

@eichelberg / @jriesmeier How about this one ?

I am still not happy with it: At least on Linux, it is not possible to safely use a static buffer to store a path. The PATH_MAX constant exists, but the documentation warns against using it. The problem is that at least on Linux, the maximum path length depends on the filesystem used, and is not a hard-coded kernel limit. Therefore, I would suggest that you allocate that buffer dynamically with malloc() and add the corresponding free() call to the OFiconv_cleanup(), which is automatically called when an application terminates (search for OFiconvCleanupHelper in module ofstd for the ugly details).

[eichelberg]

This looks good now. I just merged this into our testing branch. It should appear in the public repository in a few days.