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

minor cleanup for code scan #194

Merged
merged 15 commits into from
Sep 21, 2024
Merged

minor cleanup for code scan #194

merged 15 commits into from
Sep 21, 2024

Conversation

liquidaty
Copy link
Owner

@liquidaty liquidaty commented Sep 20, 2024

  • Minor cleanup for code scan
  • Use clang-format-15
  • Format source files if required (missing ones)

@iamazeem
Copy link
Collaborator

@liquidaty: Please review.
The formatting and tests issues have been fixed. Thanks!

app/select.c Dismissed Show dismissed Hide dismissed
iamazeem and others added 2 commits September 20, 2024 18:58
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@iamazeem
Copy link
Collaborator

CodeQL Code Scanning Report for this branch:
https://github.com/liquidaty/zsv/security/code-scanning?query=is%3Aopen+branch%3Acleanup-20240919

Apparently, the first two are fixable.
The last two may also be fixed by writing our own function to encode SQL query. 🤔
The rest third-party ones, coming from SQLite3 source, may be marked as "won't fix".

@iamazeem
Copy link
Collaborator

Just verified with clang-format 18.1.3 on https://clang-format-configurator.site with our config that this line:

if(err)

is formatting correctly i.e.:

-if(err)
+if (err)

@iamazeem
Copy link
Collaborator

Looked into these CodeQL issues:

Both seem to be false positives.

Tried setting the respective pointer to NULL. No effect.
Tried alternate if-else flows for both cases but they had no effect:

zsv.c

  parser->fixed.offsets = calloc(count, sizeof(*parser->fixed.offsets));
  if (parser->fixed.offsets) {
    parser->fixed.count = count;
    for (unsigned i = 0; i < count; i++)
      parser->fixed.offsets[i] = offsets[i];
  } else {
    fprintf(stderr, "Out of memory!\n");
    return zsv_status_memory;
  }

select.c

        free(data.fixed.offsets);
        data.fixed.offsets = malloc(data.fixed.count * sizeof(*data.fixed.offsets));
        if (data.fixed.offsets) {
          size_t count = 0;
          const char *start = argv[arg_i];
          for (const char *end = argv[arg_i];; end++) {
            if (*end == ',' || *end == '\0') {
              if (sscanf(start, "%zu,", &data.fixed.offsets[count++]) != 1) {
                stat = zsv_printerr(1, "Invalid offset: %.*s\n", end - start, start);
                break;
              } else if (*end == '\0')
                break;
              else {
                start = end + 1;
                if (*start == '\0')
                  break;
              }
            }
          }
        } else {
          fprintf(stderr, "Out of memory!\n");
          return zsv_status_memory;
        }

Tested with "out of memory" check first too. No effect at all.

Also, on these checks, there's this warning:

WARNING: This check is an approximation, so some results may not be actual defects in the program. It is not possible in general to compute the values of pointers without running the program with all input data.

Given above, looks like these can safely be marked/dismissed as false positives from their respective links.

if (i > 0)
result = zsv_select_column_index_selection_type_single;
} else {
k = sscanf((const char *)arg, "%u-%n", &i, &n);
if (k && n == (int)strlen((const char *)arg)) {
if (k == 1 && n >= 0 && (size_t)n == strlen((const char *)arg)) {
Copy link
Collaborator

@iamazeem iamazeem Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if n will ever be 0 for these cases. 🤔
Checking n against strlen() seems to be enough.
Please see these examples:

n is never 0 here.

Also, the negative lower-bounded case (-5) seems to be handled as single index.
It's being read and wrapped around as its type is unsigned int.

Here's a sample CLI interaction:

Screenshot from 2024-09-21 16-16-33

@liquidaty liquidaty merged commit c011cf5 into main Sep 21, 2024
5 checks passed
@iamazeem iamazeem deleted the cleanup-20240919 branch September 22, 2024 06:50
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

Successfully merging this pull request may close these issues.

2 participants