Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logging: Fix easylogging++ init with blank config #9809

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iamamyth
Copy link

The upstream version of el::base::TypedConfigurations::unsafeGetConfigByRef accesses uninitialized memory if a key doesn't exist. Commit b2c59af patched the library to throw in this case, avoiding the invalid access, but the more suitable pattern, both logically (configure with a default-initialized config shouldn't throw), and as evidenced by the behavior of unsafeGetConfigByVal, would be to return a const reference to a default-initialized value with static storage duration.

The upstream version of el::base::TypedConfigurations::unsafeGetConfigByRef
accesses uninitialized memory if a key doesn't exist.
Commit b2c59af patched the library to
throw in this case, avoiding the invalid access, but the more suitable
pattern, both logically, and as evidenced by the behavior of
unsafeGetConfigByVal, would be to return a const reference to a
default-initialized value with static storage duration.
@iamamyth
Copy link
Author

iamamyth commented Feb 20, 2025

For the avoidance of doubt: The library seems quite clearly to intend the "blank config" case to work. For example, in unsafeValidateFileRolling, it checks:

  base::type::fstream_t* fs = unsafeGetConfigByRef(level, &m_fileStreamMap, "fileStream").get();
  if (fs == nullptr) {
    return true;
  }

The structure and behavior of unsafeGetConfigByVal (which logs an error for a missing key and returns a default-initialized object) exactly matching unsafeGetConfigByRef (with the exception of the missing return) evidences a fairly clear intent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants