Re: ping: [patchv2] Fix vDSO l_name for GDB's: Can't read pathname for load map: Input/output error.

["Carlos O'Donell" <carlos at redhat dot com>]

On 11/07/2013 01:54 PM, Jan Kratochvil wrote:
> On Thu, 07 Nov 2013 18:21:24 +0100, Carlos O'Donell wrote:
>> Under what conditions do we have L_NAME pointing to an empty string?
>> * ld.so only?
> no

OK, only executable and vdso for now.

>> * dlopen of ""?
> no

I double checked that dlmopen of NULL doesn't allow you to trigger
_dl_new_object with "" as the file. We don't allow dlmopen of the
global namespace with LM_ID_NEWLM because it doesn't make sense
since such a namespace would be empty. We may allow this in the 
future IMO, but that's is not material to this discussion. Either
way it wouldn't break things, but I wanted to know we had considered
all callers.

>> * Is it possible with other libraries? 
> I do not think so.

OK.

> It was in the original mail post:
> 
> # This condition gets applied for these callers:
> # elf/rtld.c:dl_main():1128:
> #       /* Create a link_map for the executable itself.
> #          This will be what dlopen on "" returns.  */
> #       main_map = _dl_new_object ((char *) "", "", lt_executable, NULL,
> #                                  __RTLD_OPENEXEC, LM_ID_BASE);
> # elf/setup-vdso.h:setup_vdso():32:
> #   struct link_map *l = _dl_new_object ((char *) "", "", lt_library, NULL,
> #                                        0, LM_ID_BASE);
> 
> It was happening for vDSO in the past but currently L_NAME gets replaced later
> in the code.  So nowadays it happens in practice only for the main executable.

OK, I agree after review.

>> Why does that imply the empty string is on a read-only page? 
> 
> Because the code uses '(char *) ""' which points into ld.so .rodata section.

But _dl_new_object does not know this so you're code makes an assumption
which we should state in a comment.
 
>> * Is this something that always happens?
> Yes.

Agreed.

>> * Is this something that only happens with ld.so?
> 
> I do not understand this question.  'ld.so' is related to this problem only by
> the fact that the read-only page belongs to the mapped ld.so binary.  But it
> is unrelated for the link_map entry for ld.so.

I thought that previously we used to call _dl_new_object with "" for ld.so,
but I think recently we fixed all of that by using rtld_progname everywhere,
but I could be wrong.

Either way you've cleared up that this happens only for the main executalbe
map and the VDSO (though that's later fixed up).
 
>> Does the entire glibc testsuite pass after this change?
>> * It might change the output of LD_DEBUG=all?
>> * It might change the output of sotruss-lib used for testing?
> 
> Primarily I do not see how it could change anything (besides the GDB core file
> load).  It is still "" even after the patch.  Just that "" is located
> elsewhere.  IIRC I was doing the testing, such as verifying rpmbuild output
> (rpmbuild runs the testsuite) so I am reluctant to set it up and run again.
> 
> Do you have some suspection what could cause any problems?

No, but patch submitters are required to run the glibc testcase and look
for regressions (it needs to be clean before and after).

Did you see any regressions?

>> Do other tools run OK after this change?
>> * Valgrind has no problems?
>> * gdb itself has no problems? (I assume it doesn't)
>> * ldd shows now difference? (Related to LD_DEBUG=all above, but in trace mode)
> 
> IIRC I was running mock chroot with that patched glibc and I primarily tested
> GDB is happy.  As written above I cannot imagine how any tool besides GDB
> loading a core file could find a difference.

Sounds good.

I am not requiring you to test these things, but I'm trying to learn
what kind of testing you did. You didn't state what kind of testing
you did and I don't want to assume.

> Sure regressions happen in unpredictable way but this is why you do the
> qualified review, why regular regression tests happen and why there are
> development vs. stable branches.

I agree, and yet we still deploy buggy software :-)

-  new->l_name = realname;
+  /* Ensure empty strings from readonly memory are stored in a written page so
+     the string gets dumped into the core file.  */
+  new->l_name = *realname ? realname : newname->name + libname_len - 1;

I suggest the following more verbose comment:

/* When we create the executable link map, or a VDSO link map, we start
   with "" for the l_name. In these cases "" points to ld.so rodata
   and won't get dumped during core file generation. Therefore to assist
   gdb and to create more self-contained core files we adjust l_name to
   point at the newly allocated copy (which will get dumped) instead of 
   the ld.so rodata copy.  */

OK with those changes, and confirmation that no regressions were seen in the
glibc testsuite.

Cheers,
Carlos.