Bug 104793 - -Wanalyzer-write-to-const and -Wanalyzer-write-to-string-literal should respect attribute((access, write)
Summary: -Wanalyzer-write-to-const and -Wanalyzer-write-to-string-literal should respe...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: analyzer (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: ---
Assignee: David Malcolm
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-03-04 19:57 UTC by David Malcolm
Modified: 2022-03-10 14:23 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Malcolm 2022-03-04 19:57:17 UTC
As of GCC 10 (I believe):
  __attribute__ ((access (MODE, REF_INDEX[, SIZE_INDEX])))

can be used to mark function decls with info on what buffers they access:
  https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

Given the following:

#include <stdio.h>
#include <features.h>

ssize_t getrandom (void *__buffer, size_t __length,
                   unsigned int __flags)
    __attribute__ ((access (__write_only__, 1, 2)));

#define GRND_RANDOM 0x02

const char *test = "test";

int main(void)
{
	const char buf[5] = { 0 };

	if (getrandom(test, sizeof(buf), GRND_RANDOM))
		printf("%s\n", buf);

	return 0;
}


When it runs, this is in the strace:

  getrandom(0x402010, 5, GRND_RANDOM)     = -1 EFAULT (Bad address)

trunk (for gcc 12) correctly complains about:

test.c: In function ‘main’:
test.c:16:23: warning: passing argument 1 of ‘getrandom’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   16 |         if (getrandom(test, sizeof(buf), GRND_RANDOM))
      |                       ^~~~
test.c:4:26: note: expected ‘void *’ but argument is of type ‘const char *’
    4 | ssize_t getrandom (void *__buffer, size_t __length,
      |                    ~~~~~~^~~~~~~~

However, -fanalyzer doesn't complain.  It would be good if the analyzer took account of the access attribute to notice the attempt to write to the string literal "test", and emitted -Wanalyzer-write-to-string-literal on the above code.

Note that glibc doesn't yet mark getrandom with that attribute:
  https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/sys/random.h

(Would be nice to statically bounds-check the accesses as well, but that's a different issue)
Comment 1 David Malcolm 2022-03-09 20:23:11 UTC
See also PR analyzer/104860, which covers this for -Wanalyzer-possible-null-argument and -Wanalyzer-null-argument.
Comment 2 GCC Commits 2022-03-10 14:05:06 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:b6eaf90c64f91553c8002f6ee401785a8bc6f94c

commit r12-7595-gb6eaf90c64f91553c8002f6ee401785a8bc6f94c
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Thu Mar 10 09:04:03 2022 -0500

    analyzer: check for writes to consts via access attr [PR104793]
    
    This patch extends:
      -Wanalyzer-write-to-const
      -Wanalyzer-write-to-string-literal
    so that they will check for __attribute__ ((access, ....) on calls to
    externally-defined functions, and complain about read-only regions
    pointed to by arguments marked with a "write_only" or "read_write"
    attribute.
    
    gcc/analyzer/ChangeLog:
            PR analyzer/104793
            * region-model.cc
            (region_model::check_external_function_for_access_attr): New.
            (region_model::handle_unrecognized_call): Call it.
            * region-model.h
            (region_model::check_external_function_for_access_attr): New decl.
            (region_model::handle_unrecognized_call): New decl.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/104793
            * gcc.dg/analyzer/write-to-const-2.c: New test.
            * gcc.dg/analyzer/write-to-function-1.c: New test.
            * gcc.dg/analyzer/write-to-string-literal-2.c: New test.
            * gcc.dg/analyzer/write-to-string-literal-3.c: New test.
            * gcc.dg/analyzer/write-to-string-literal-4.c: New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Comment 3 GCC Commits 2022-03-10 14:10:33 UTC
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:c65d3c7f9dade18826d1a28d71b98c7cdc3e4300

commit r12-7596-gc65d3c7f9dade18826d1a28d71b98c7cdc3e4300
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Thu Mar 10 09:09:40 2022 -0500

    analyzer: add notes to write-to-const/string from access attr [PR104793]
    
    The previous patch extended
      -Wanalyzer-write-to-const
      -Wanalyzer-write-to-string-literal
    to make use of __attribute__ ((access, ....), but the results could be
    inscrutable.
    
    This patch adds notes to such diagnostics to give the user a reason for
    why the analyzer is complaining.
    
    Example output:
    
    test.c: In function 'main':
    test.c:15:13: warning: write to string literal [-Wanalyzer-write-to-string-literal]
       15 |         if (getrandom((char *)test, sizeof(buf), GRND_RANDOM))
          |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      'main': event 1
        |
        |   15 |         if (getrandom((char *)test, sizeof(buf), GRND_RANDOM))
        |      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        |      |             |
        |      |             (1) write to string literal here
        |
    test.c:3:5: note: parameter 1 of 'getrandom' marked with attribute 'access (write_only, 1, 2)'
        3 | int getrandom (void *__buffer, size_t __length,
          |     ^~~~~~~~~
    
    Unfortunately we don't have location information for the attributes
    themselves, just the function declaration, and there doesn't seem to be
    a good way of getting at the location of the individual parameters from
    the middle end (the C and C++ FEs both have get_fndecl_argument_location,
    but the implementations are different).
    
    gcc/analyzer/ChangeLog:
            PR analyzer/104793
            * analyzer.h (class pending_note): New forward decl.
            * diagnostic-manager.cc (saved_diagnostic::saved_diagnostic):
            Initialize m_notes.
            (saved_diagnostic::operator==): Compare m_notes.
            (saved_diagnostic::add_note): New.
            (saved_diagnostic::emit_any_notes): New.
            (diagnostic_manager::add_note): New.
            (diagnostic_manager::emit_saved_diagnostic): Call emit_any_notes
            after emitting the warning.
            * diagnostic-manager.h (saved_diagnostic::add_note): New decl.
            (saved_diagnostic::emit_any_notes): New decl.
            (saved_diagnostic::m_notes): New field.
            (diagnostic_manager::add_note): New decl.
            * engine.cc (impl_region_model_context::add_note): New.
            * exploded-graph.h (impl_region_model_context::add_note): New
            decl.
            * pending-diagnostic.h (class pending_note): New.
            (class pending_note_subclass): New template.
            * region-model.cc (class reason_attr_access): New.
            (check_external_function_for_access_attr): Add class
            annotating_ctxt and use it when checking region.
            (noop_region_model_context::add_note): New.
            * region-model.h (region_model_context::add_note): New vfunc.
            (noop_region_model_context::add_note): New decl.
            (class region_model_context_decorator): New.
            (class note_adding_context): New.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/104793
            * gcc.dg/analyzer/write-to-const-2.c: Add dg-message directives
            for expected notes.
            * gcc.dg/analyzer/write-to-function-1.c: Likewise.
            * gcc.dg/analyzer/write-to-string-literal-2.c: Likewise.
            * gcc.dg/analyzer/write-to-string-literal-3.c: Likewise.
            * gcc.dg/analyzer/write-to-string-literal-4.c: Likewise.
            * gcc.dg/analyzer/write-to-string-literal-5.c: New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>
Comment 4 David Malcolm 2022-03-10 14:23:19 UTC
Should be fixed by the above commits.