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

Null pointer dereferences #300

Open
Bogdanisar opened this issue Apr 23, 2022 · 0 comments
Open

Null pointer dereferences #300

Bogdanisar opened this issue Apr 23, 2022 · 0 comments

Comments

@Bogdanisar
Copy link
Contributor

Hi!
I've run Cppcheck for static code analysis on the master branch of this project. I've found the following 3 code snippets which may contain either a redundant check or a NULL dereference:

1) At src/onion/onion.c:860:7:

      onion_listen_point *https = onion_https_new();
      https->server = onion;
      if (NULL == https) {
        ONION_ERROR
            ("Could not promote from HTTP to HTTPS. Certificate not set.");
      }
      https->port = port;
      https->hostname = hostname;

If the condition NULL == https can be true, then https->server is a NULL pointer dereference. Statement https->server = onion; was added in commit eaee81e03 , but before that commit the if condition was performed before the assignments. Maybe moving https->server = onion; after the condition is a good fix here?

2) At src/onion/sessions_redis.c:122:38:

  if (p == NULL) {
    redisReply *reply = redisCommand(p->context, "HDEL SESSIONS %b", session_id,
                                     strlen(session_id));
    ...
  } else {
    const char *json = onion_block_data(bl);
    redisReply *reply =
        redisCommand(p->context, "HSET SESSIONS %b %b", session_id,
                     strlen(session_id), json, strlen(json));
    ...
  }

Both code branches execute redisCommand(p->context, ...), but p is NULL for the first branch, so p->context will be a NULL pointer dereference.

3) At src/onion/poller.c:453:10:

  if (el && el->fd == fd) {
    ...
  }
  while (el->next) {
    ...
  }

If el can be NULL, then the statement while (el->next) will be a NULL pointer dereference. If not, the condition el && from if (el && el->fd == fd) is redundant.

YggdrasiI added a commit to YggdrasiI/onion that referenced this issue Oct 17, 2022
This commit resolves three null pointer dereferences mentioned by
Bogdanisar.

Comments about changes:

1) onion.c
 Add return statement if promotion to https had failed
 and set member variable only in !=NULL case.

2) Here, I'm unsure if my change is correct.

  I assume that the NULL-check is using the wrong variable!
  First of all, a check of the bl variable is missing. This indicates
  that (bl == NULL) would be the correct check.

  Moreover, the redis session probably should be deleted if the data
  dict is empty, too. Thus, I changed the line to
  if (onion_dict_count(data) == 0 || bl == NULL)

  Other variants would be
  if (bl == NULL)
  if (data == NULL || bl == NULL )

3) poller.c:
  If added a NULL check because poller->head is initialized
  with NULL in onion_poller_new.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant