Bug 92382 - variable double-definition in routine replace_filename_variables of libgcc/libgcov-driver-system.c
Summary: variable double-definition in routine replace_filename_variables of libgcc/li...
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: gcov-profile (show other bugs)
Version: 10.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-05 16:22 UTC by qinzhao
Modified: 2020-01-30 10:05 UTC (History)
1 user (show)

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 qinzhao 2019-11-05 16:22:35 UTC
In the routine replace_filename_variables of libgcc/libgcov-driver-system.c:

145 static char *
146 replace_filename_variables (char *filename)
147 {
148   char buffer[16];
149   char empty[] = "";
150   for (char *p = filename; *p != '\0'; p++)
151     {
152       unsigned length = strlen (filename);
153       if (*p == '%' && *(p + 1) != '\0')
154         {
155           unsigned start = p - filename;
156           p++;
157           char *replacement = NULL;
158           switch (*p)
159             {
160             case 'p':
161               sprintf (buffer, "%d", getpid ());
162               replacement = buffer;
163               p++;
164               break;
165             case 'q':
166               if (*(p + 1) == '{')
167                 {
168                   p += 2;
169                   char *e = strchr (p, '}');
170                   if (e)
171                     {
172                       *e = '\0';
173                       replacement = getenv (p);
174                       if (replacement == NULL)
175                         replacement = empty;
176                       p = e + 1;
177                     }
178                   else
179                     return filename;
180                 }
181               break;
182             default:
183               return filename;
184             }
185 
186           /* Concat beginning of the path, replacement and
187              ending of the path.  */
188           unsigned end = length - (p - filename);
189           unsigned repl_length = replacement != NULL ? strlen (replacement) : 0;
190 
191           char *buffer = (char *)xmalloc (start + end + repl_length + 1);
192           char *buffer_ptr = buffer;
193           buffer_ptr = (char *)memcpy (buffer_ptr, filename, start);
194           buffer_ptr += start;
195           if (replacement != NULL)
196             buffer_ptr = (char *)memcpy (buffer_ptr, replacement, repl_length);
197           buffer_ptr += repl_length;
198           buffer_ptr = (char *)memcpy (buffer_ptr, p, end);
199           buffer_ptr += end;
200           *buffer_ptr = '\0';
201 
202           free (filename);
203           filename = buffer;
204           p = buffer + start + repl_length;
205         }
206     }
207 
208   return filename;
209 }

The major issue in the above code is:  the "buffer" is redefined: 

At line 148, "buffer" is defined outside the loop, the intend usage of this 
"buffer" is at line 161 and 162 to hold the process id string for the 
replacement of "%p"; 

However, At line 191, inside this same loop, "buffer" is defined again and 
initialized to hold the new name of the whole filename, and the lifetime of 
this "buffer" will overwrite the other "buffer" defined outside of the loop.   

the fix to this problem is to rename the second "buffer" of line 183 to 
another different name.
Comment 1 Jakub Jelinek 2019-11-05 17:12:47 UTC
Why is this a major issue?  Just variable shadowing, so something that with -Wshadow* compiler will warn, but nothing more, the code is well defined, isn't it?
Comment 2 qinzhao 2019-11-05 17:43:51 UTC
(In reply to Jakub Jelinek from comment #1)
> Why is this a major issue?  Just variable shadowing, so something that with
> -Wshadow* compiler will warn, but nothing more, the code is well defined,
> isn't it?

when you print the value of "buffer" inside gdb at line 161, it's NULL, not a buffer with size 16. As a result, the sprint (buffer, "%d", get pic ()) at line 161 will write the processID to a NULL buffer, is this correct?
Comment 3 Jakub Jelinek 2019-11-05 17:48:48 UTC
Most likely just GDB doesn't handle it correctly, or a bug in what we emit as debug info for it (for -O0 it wouldn't surprise me, as we don't really track the scope of the variable).
Comment 4 Jakub Jelinek 2019-11-05 17:59:24 UTC
This boils down to
int
main ()
{
  volatile int v = 0;
  {
    v++;
    v++;
    volatile int v = 4;
    v++;
  }
}
From what I see, this is handled correctly in the generated code, so it is just the debugging issue.
Comment 5 qinzhao 2019-11-05 19:02:00 UTC
Okay, I see. thank you for explanation.
I will close this one as not a bug.

(In reply to Jakub Jelinek from comment #4)
Comment 6 Jakub Jelinek 2019-11-05 19:04:19 UTC
Feel free to open an issue against GDB or GCC< wherever the debug info issue is e.g. for the #c4 testcase.  Because certainly I see 0 as the value of v even when it should be 1 or 2.
Comment 7 qinzhao 2019-11-05 22:00:13 UTC
I have just created a bug to record the debugging issue:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92386

(In reply to Jakub Jelinek from comment #6)
> Feel free to open an issue against GDB or GCC< wherever the debug info issue
> is e.g. for the #c4 testcase.  Because certainly I see 0 as the value of v
> even when it should be 1 or 2.
Comment 8 Martin Liška 2020-01-30 10:05:52 UTC
As the debug issue was created, I'm closing this.