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.

Sign up for GitHub

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

Jump to bottom

Possible NULL pointer dereference #149

Closed
Rob-McKay opened this issue May 2, 2023 · 9 comments
Closed

Possible NULL pointer dereference #149

Rob-McKay opened this issue May 2, 2023 · 9 comments
Assignees
@eyalroz
Labels
resolved-on-develop A changeset fixing this issue has been commiutted to the development branch

Comments

@Rob-McKay
Copy link

Hi,

When I build printf with GCC 12.2 from ARM I get the following error

  FAILED: E70_Bootloader/CMakeFiles/E70_Bootloader.dir/__/printf/src/printf/printf.c.obj 
  @arm-gnu-toolchain-12.2.rel1-mingw-w64-i686-arm-none-eabi\bin\arm-none-eabi-gcc.exe -DSAME70=1 -D__SAME70Q21B__ -IE70_Boot_loader/E70_Bootloader/. -IE70_Boot_loader/E70_Bootloader/../printf/src -g3 -Og -Wall -pedantic -DDEBUG -mthumb -mcpu=cortex-m7 -mfloat-abi=hard -mfpu=fpv5-d16 -specs=nano.specs -fcallgraph-info=su,da -fstack-protector-strong -mno-unaligned-access -Wall -Wextra -Wpedantic -Werror -Wformat=2 -Wcast-align -Wsign-conversion -Wdouble-promotion -Wshadow -Wstrict-prototypes -Wold-style-definition -Wpadded -Wlogical-op -fanalyzer -fno-builtin-printf -Wno-padded -MD -MT E70_Bootloader/CMakeFiles/E70_Bootloader.dir/__/printf/src/printf/printf.c.obj -MF E70_Bootloader\CMakeFiles\E70_Bootloader.dir\__\printf\src\printf\printf.c.obj.d -o E70_Bootloader/CMakeFiles/E70_Bootloader.dir/__/printf/src/printf/printf.c.obj -c E70_Boot_loader/printf/src/printf/printf.c
  E70_Boot_loader/printf/src/printf/printf.c: In function 'putchar_via_gadget':
E70_Boot_loader\printf\src\printf\printf.c(360,31): error GAD6F4944: dereference of NULL '0' [CWE-476] [-Werror=analyzer-null-dereference]
    360 |     gadget->buffer[write_pos] = c;
        |     ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
    'fctprintf': events 1-2
      |
      | 1452 | int fctprintf(void (*out)(char c, void* extra_arg), void* extra_arg, const char* format, ...)
      |      |     ^~~~~~~~~
      |      |     |
      |      |     (1) entry to 'fctprintf'
      |......
      | 1456 |   const int ret = vfctprintf(out, extra_arg, format, args);
      |      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |      |                   |
      |      |                   (2) calling 'vfctprintf' from 'fctprintf'
      |
      +--> 'vfctprintf': events 3-4
             |
             | 1419 | int vfctprintf(void (*out)(char c, void* extra_arg), void* extra_arg, const char* format, va_list arg)
             |      |     ^~~~~~~~~~
             |      |     |
             |      |     (3) entry to 'vfctprintf'
             | 1420 | {
             | 1421 |   output_gadget_t gadget = function_gadget(out, extra_arg);
             |      |                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             |      |                            |
             |      |                            (4) calling 'function_gadget' from 'vfctprintf'
             |
             +--> 'function_gadget': events 5-6
                    |
                    |  408 | static inline output_gadget_t function_gadget(void (*function)(char, void*), void* extra_arg)
                    |      |                               ^~~~~~~~~~~~~~~
                    |      |                               |
                    |      |                               (5) entry to 'function_gadget'
                    |  409 | {
                    |  410 |   output_gadget_t result = discarding_gadget();
                    |      |                            ~~~~~~~~~~~~~~~~~~~
                    |      |                            |
                    |      |                            (6) calling 'discarding_gadget' from 'function_gadget'
                    |
                    +--> 'discarding_gadget': event 7
                           |
                           |  385 | static inline output_gadget_t discarding_gadget(void)
                           |      |                               ^~~~~~~~~~~~~~~~~
                           |      |                               |
                           |      |                               (7) entry to 'discarding_gadget'
                           |
                         'discarding_gadget': event 8
                           |
                           |  390 |   gadget.buffer = NULL;
                           |      |                 ^
                           |      |                 |
                           |      |                 (8) 'gadget.buffer' is NULL
                           |
                    <------+
                    |
                  'function_gadget': event 9
                    |
                    |  410 |   output_gadget_t result = discarding_gadget();
                    |      |                            ^~~~~~~~~~~~~~~~~~~
                    |      |                            |
                    |      |                            (9) returning to 'function_gadget' from 'discarding_gadget'
                    |
             <------+
             |
           'vfctprintf': events 10-11
             |
             | 1421 |   output_gadget_t gadget = function_gadget(out, extra_arg);
             |      |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             |      |                            |
             |      |                            (10) returning to 'vfctprintf' from 'function_gadget'
             | 1422 |   return vsnprintf_impl(&gadget, format, arg);
             |      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             |      |          |
             |      |          (11) calling 'vsnprintf_impl' from 'vfctprintf'
             |
             +--> 'vsnprintf_impl': events 12-13
                    |
                    | 1387 | static int vsnprintf_impl(output_gadget_t* output, const char* format, va_list args)
                    |      |            ^~~~~~~~~~~~~~
                    |      |            |
                    |      |            (12) entry to 'vsnprintf_impl'
                    |......
                    | 1391 |   format_string_loop(output, format, args);
                    |      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                    |      |   |
                    |      |   (13) calling 'format_string_loop' from 'vsnprintf_impl'
                    |
                    +--> 'format_string_loop': events 14-19
                           |
                           | 1074 | static inline void format_string_loop(output_gadget_t* output, const char* format, va_list args)
                           |      |                    ^~~~~~~~~~~~~~~~~~
                           |      |                    |
                           |      |                    (14) entry to 'format_string_loop'
                           |......
                           | 1083 |   while (*format)
                           |      |          ~          
                           |      |          |
                           |      |          (15) following 'true' branch...
                           | 1084 |   {
                           | 1085 |     if (*format != '%') {
                           |      |     ~~ ~            
                           |      |     |  |
                           |      |     |  (17) following 'true' branch...
                           |      |     (16) ...to here
                           | 1086 |       // A regular content character
                           | 1087 |       putchar_via_gadget(output, *format);
                           |      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                           |      |       |
                           |      |       (18) ...to here
                           |      |       (19) calling 'putchar_via_gadget' from 'format_string_loop'
                           |
                           +--> 'putchar_via_gadget': events 20-30
                                  |
                                  |  345 | static inline void putchar_via_gadget(output_gadget_t* gadget, char c)
                                  |      |                    ^~~~~~~~~~~~~~~~~~
                                  |      |                    |
                                  |      |                    (20) entry to 'putchar_via_gadget'
                                  |......
                                  |  350 |   if (write_pos >= gadget->max_chars) {
                                  |      |      ~              
                                  |      |      |
                                  |      |      (21) following 'false' branch...
                                  |......
                                  |  353 |   if (gadget->function != NULL) {
                                  |      |   ~~ ~              
                                  |      |   |  |
                                  |      |   |  (23) following 'false' branch...
                                  |      |   (22) ...to here
                                  |......
                                  |  360 |     gadget->buffer[write_pos] = c;
                                  |      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                  |      |     |     |       |           |
                                  |      |     |     |       |           (30) dereference of NULL '*gadget.buffer + *gadget.pos'
                                  |      |     |     |       (26) 'gadget.buffer' is NULL
                                  |      |     |     |       (28) 'gadget.buffer' is NULL
                                  |      |     |     |       (29) 'gadget.buffer' is NULL
                                  |      |     |     (25) 'gadget.buffer' is NULL
                                  |      |     |     (27) 'gadget.buffer' is NULL
                                  |      |     (24) ...to here
                                  |
@mickjc750
Copy link

It looks like you are calling fctprintf() with a NULL instead of your output function. Is it your intention to discard the output?

@Rob-McKay
Copy link
Author

Rob-McKay commented May 2, 2023 •
edited
Loading

I'm not actually calling anything yet.

This is generated by the GCC 12.2Rel1 when it is doing its analysis of the printf.c file.

I think that the underlying issue is that the out parameter to vfctprintf is never checked to ensure that it is not NULL, so the code path indicated by the compiler could happen.

@eyalroz
Copy link
Owner

eyalroz commented May 2, 2023 •
edited
Loading

Hmm. So, the assignment into a NULL buffer can happen if the function gadget was created with a NULL function pointer. This only happens if the user invoked fctprintf() with a NULL function pointer. This library does not, generally, protect against dumb inputs. So, you'll also get a null pointer dereference if you write printf(NULL), because I execute while(*format), in the format_string_loop().

So, basically - it's not a bug, just a potential foot-gun, which is a design decision older than my involvement in this project.

@Rob-McKay
Copy link
Author

So, basically - it's not a bug, just a potential foot-gun, which is a design decision older than my involvement in this project.

It also prevents compiling on GCC 12.2Rel1 with -Werror.

The warning goes away if I check the out parameter for NULL in vfctprintf but I'm not sure what the return value should be in that case.

@eyalroz
Copy link
Owner

eyalroz commented May 3, 2023

@Rob-McKay : Is there a preprocessor macro which I can use to identify this compiler? Because GCC 12.2 on x86_64 doesn't complain about the issue. If I could pin down your compiler, I could add a warning suppression at that line.

@Rob-McKay
Copy link
Author

@eyalroz I have raised pull request #150 to fix this issue

@eyalroz
Copy link
Owner

eyalroz commented May 4, 2023

The issue is a excessive warning with your compiler. I'll be happy to take a PR to address that issue - but certainly not to hurt performance in the innermost loop.

@Rob-McKay
Copy link
Author

@eyalroz: I have found out what causes GCC to generate the warning. If you add the -fanalyzer compiler option you will also see the warning on x86_64.

@eyalroz
Copy link
Owner

eyalroz commented May 8, 2023

So, let me just add an outer check in vfctprintf().

eyalroz added a commit that referenced this issue May 8, 2023
@eyalroz
Fixes #149: When the user specifies a NULL output-function pointer, g…
c2e239d 
…ive up immediately and avoid NULL-dereferencing situations (which GCC's static analyzer complains about)
@eyalroz eyalroz self-assigned this May 8, 2023
@eyalroz eyalroz added the resolved-on-develop A changeset fixing this issue has been commiutted to the development branch label May 8, 2023
@eyalroz eyalroz mentioned this issue May 8, 2023
Closed
eyalroz added a commit that referenced this issue Jul 19, 2024
@eyalroz
Fixes #149: When the user specifies a NULL output-function pointer, g…
28addc7 
…ive up immediately and avoid NULL-dereferencing situations (which GCC's static analyzer complains about)
phillipjohnston pushed a commit to embeddedartistry/printf that referenced this issue Mar 3, 2025
@eyalroz @phillipjohnston
Fixes eyalroz#149: When the user specifies a NULL output-function poi…
3353336 
…nter, give up immediately and avoid NULL-dereferencing situations (which GCC's static analyzer complains about)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eyalroz eyalroz

Labels
resolved-on-develop A changeset fixing this issue has been commiutted to the development branch
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mickjc750 @eyalroz @Rob-McKay