Thanks for the patch.

Would it be possible to make a version without all of the whitespace changes?

Secondly, you are breaking the strict static linking by adding -lc. That is a big no-no. You should be using VG_(sysctlbyname) rather than sysctlbyname, and similarly you should be using vki_ data structures, adding to include/vki/vki-freebsd.h if necessary
 (though that is not done 100% of the time).

Created attachment 150313 [details]
Quick patch II: Le retour

All done! Sorry about, I'm completely new to this codebase 😅

Is there a comprehensive document somewhere about the dos and don'ts/what's convention for future reference?

Thanks again. Do you know where the memory that memcheck is complaining about comes from?

I'll do some tests with konsole/xterm and see what they do.

As for documentation, README_DEVELOPERS, README_DEVELOPERS_processes and README.freebsd are a good start. There's some good stuff in docs/internals. After that, there are the mailing lists and their archives.

For "live" questions, #valgrind-dev in Libera.Chat IRC is the best (Valgrind is very old-school).

Thank you, I'll check out the IRC next time, I must've missed it when looking over the site ;)

> Do you know where the memory that memcheck is complaining about comes from?

Pardon, what do you mean by this? When running a simple test program which calls setproctitle, I'm not getting any leaks or addressing errors...

> Pardon, what do you mean by this? When running a simple test program which
> calls setproctitle, I'm not getting any leaks or addressing errors...

I haven't done any testing yest, but looking at the source I see that setproctitle first tries auxv. When an application runs under Valgrind it doesn't get a full auxv, rather it gets one that has been sanitized by Valgrind without any dynamic memory. So presume the _elf_aux_info call will fail and the fallback is to

	if (ps_strings == NULL) {
		len = sizeof(ul_ps_strings);
		if (sysctlbyname("kern.ps_strings", &ul_ps_strings, &len, NULL,
		    0) == -1)
			return (NULL);
		ps_strings = (struct ps_strings *)ul_ps_strings;
	}

This is getting a pointer to some memory. My question is where does that memory come from? And I presume (again) that it causes problems because it came from somewhere and memcheck didn't see it being allocated.

Also this code does look a bit leaky. The buffers allocated by malloc never get freed, so if you run with --leak-check=full --show-reachable=yes I expect that memcheck will complain about that memory. That's not a big issue, and it would require changes to libc, see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=259294

> So presume the _elf_aux_info call will fail

Yeah it does, that's the raison d'être of this patch.

> My question is where does that memory come from?

It should be copied over on exec ('sys/kern/kern_exec.c', that's also where the 'kern.ps_strings' sysctl is implemented) to userspace at 'PROC_PS_STRINGS(proc)' (which resides somewhere at the beginning of that process' userstack, cf 'sys/sys/exec.h').

> And I presume (again) that it causes problems because it came from somewhere and memcheck didn't see it being allocated.

I'm not entirely sure why Valgrind doesn't track this memory, but in any case there should be a guarantee that it's okay to access this memory, which is why I attempted to do what I attempted to do with this patch - ignore accesses to that memory.

Again though, there may be (and actually probably is) a much more elegant solution to this which I'm not seeing right now!

> Also this code does look a bit leaky

It indeed shouldn't be much of an issue because 'buf' & 'obuf' are both static and checked to be NULL before being allocating. I can't test with '--show-reachable=yes' until Monday, but I also expect it to make memcheck complain.

Have a nice weekend!

(In reply to obiwac@gmail.com from comment #7)

> I'm not entirely sure why Valgrind doesn't track this memory, but in any
> case there should be a guarantee that it's okay to access this memory, which
> is why I attempted to do what I attempted to do with this patch - ignore
> accesses to that memory.

How can memcheck know? It's intercepting all of the 'malloc' and 'new' family functions so it knows about them. It also created the client stack so it knows the base and also via the stack pointer in VEX the bottom of the stack.

Here's one error

==5238== Invalid read of size 8
==5238==    at 0x490B56E: ??? (in /lib/libc.so.7)
==5238==    by 0x490B778: setproctitle (in /lib/libc.so.7)
==5238==    by 0x2018C3: main (setproctitle.c:6)
==5238==  Address 0x7fffffffefe0 is not stack'd, malloc'd or (recently) free'd

This is the top of the address space
--5238:1: aspacem  50: RSVN 0800000000-7fffdfffefff 131039g ----- SmFixed
--5238:1: aspacem  51: ANON 7fffdffff000-7ffffffdefff    511m -----
--5238:1: aspacem  52: ANON 7ffffffdf000-7fffffffefff  131072 rw---
--5238:1: aspacem  53: ANON 7ffffffff000-7fffffffffff    4096 r-x--
--5238:1: aspacem  54: RSVN 800000000000-ffffffffffffffff  16383e ----- SmFixed

0x7fffffffefe0 is on the Valgrind host's stack. 

That's what I would expect. FreeBSD doesn't know about Valgrind's phoney auxv for its guest, so the sysctl interacts with the host auxv.

Valgrind doesn't track its own memory. The expectation is that host and guest memory are separate and that the guest doesn't access host memory.


> Again though, there may be (and actually probably is) a much more elegant
> solution to this which I'm not seeing right now!

The alternative that I see is to add special handling for kern.ps_strings similar to what is being done for kern.userstack

commit 9f27d8fbc733165d7b1afbc86f1b3cdcd3837cd1
Author: Paul Floyd <pjfloyd@wanadoo.fr>
Date:   Sun Jul 3 13:05:54 2022 +0200

    Bug-456171 [PATCH] FreeBSD: Don't record address errors when accessing the 'kern.ps_strings' sysctl struct
    
    There is quite a lot of stuff here.
    
    The problem is that setproctitle and kern.ps_strings were using the Valgrind host auxv
    rather than the guest. The proposed patch would have just ignored those memory ranges.
    
    I've gone a fair bit further than that
    1. refactored the initimg code for building the client auxv. Previously we were
       simply ignoring any non-scalar entries. Now we copy most of thse as well.
       That means that 'strtab' built on the client stack no longet only contains
       strings, at can also now contain binary structures. Note I was a bit
       concerned that there may be some alignment issues, but I haven't seen any
       problems so far.
    2. Added intercepts to sysctl and sysctlbyname for kern.ps_strings, then find
       AT_PS_STRINGS from the client auxv that is now usable from step 1.
    3. Some refactoring of sysctl and sysctlbyname syscall wrappers. More to do
       there!
    4. Added a setproctitle testcase (that also tests the sysctls).
    5. Updated the auxv testcase now that more AT_* entries are handled.

Awesome! Thank you!