Le Mon, 20 Jun 2016 15:07:52 +0300, "Dmitry V. Levin" <l...@altlinux.org> a écrit :
> On Sat, Jun 18, 2016 at 07:13:54PM +0200, Antoine Damhet wrote: > [...] > > +static int > > +decode_binder_commands_buffer(struct tcb *tcp, uintptr_t buffer, > > size_t pos, > > + size_t size, const struct xlat *x, const char *str) > > +{ > > + if (size < sizeof(uint32_t)) { > > + tprints("[]"); > > + return 0; > > + } > > Wouldn't it be better to print the address of buffer instead? > Will do. > > + > > + if (abbrev(tcp)) { > > + tprints("[...]"); > > + return 0; > > + } > > + > > + uint32_t type; > > + tprints("["); > > + > > + goto print_one_commands_buffer; > > + while (pos + sizeof(uint32_t) <= size) { > > Why the check is skipped for the first time? > I forgot that pos wasn't initialized a 0. > > + tprints(", "); > > + > > +print_one_commands_buffer: > > + if (umove(tcp, buffer + pos, &type)) > > What's going to be fetched when buffer + pos overflows? > > > + return 1; > > What's going to be printed when this umove call fails? > I will print ']'. > > + if (_IOC_SIZE(type) > 0) { > > + tprints("["); > > + printxval(x, type, str); > > + tprints(", "); > > + if (pos + sizeof(type) + _IOC_SIZE(type) > > <= size) > > + printstr(tcp, buffer + pos + > > sizeof(type), > > + _IOC_SIZE(type)); > > + tprints("]"); > > What's going to be printed when this condition is false? > What should I print ? > > + } else > > + printxval(x, type, str); > > + pos += sizeof(uint32_t) + _IOC_SIZE(type); > > What's going to happen when pos overflows? > The umove should have failed before but should I still check it ? > > + } > > + > > + tprints("]"); > > + return 0; > > +} > > + > > +static int > > +decode_binder_write_read(struct tcb *tcp, const long addr) > > +{ > > + struct binder_write_read wr; > > + > > + if (entering(tcp)) { > > + tprints(", "); > > + if (umove_or_printaddr(tcp, addr, &wr)) > > + return RVAL_DECODED | 1; > > + > > + tprintf("{write_size=%" PRIu64 ", > > write_consumed=%" PRIu64 > > + ", write_buffer=", > > + (uint64_t)wr.write_size, > > + (uint64_t)wr.write_consumed); > > + if (decode_binder_commands_buffer(tcp, > > wr.write_buffer, > > + wr.write_consumed, > > wr.write_size, > > + binder_driver_commands, > > "BC_???")) { > > Does the kernel take write_consumed into account? > If not, shouldn't 0 be passed to decode_binder_commands_buffer > instead, and what's the use of printing write_consumed on entering? > Yes, the kernel take write_consumed into account. > > + tprints("}"); > > + return RVAL_DECODED | 1; > > + } > > + > > + tprintf(", read_size=%" PRIu64 ", read_consumed=%" > > PRIu64 "}", > > + (uint64_t)wr.read_size, > > + (uint64_t)wr.read_consumed); > > If read_consumed is write only, what's the use of printing it on > entering? > It's not but I'm not sure how I should keep read_consumed between entering and exiting. > > + return 0; > > + } > > + > > + if (syserror(tcp) || umove(tcp, addr, &wr)) > > + return RVAL_DECODED | 1; > > + > > + tprints(" => "); > > + > > + tprintf("{write_size=%" PRIu64 ", write_consumed=%" PRIu64 > > + ", read_size=%" PRIu64 ", read_consumed=%" > > PRIu64 > > + ", read_buffer=", > > + (uint64_t)wr.write_size, > > + (uint64_t)wr.write_consumed, > > + (uint64_t)wr.read_size, > > + (uint64_t)wr.read_consumed); > > If write_size and read_size are read only, what's the use of printing > them on exiting? > I won't print them anymore. > [...] > > --- a/configure.ac > > +++ b/configure.ac > > @@ -440,6 +440,24 @@ AC_CHECK_HEADERS([linux/bpf.h], [ > > fi > > ]) > > > > +AC_CHECK_HEADERS([linux/android/binder.h], [[ #include > > <sys/types.h> +#include <linux/android/binder.h>]) > > + AC_CHECK_DECLS(m4_normalize([BC_TRANSACTION, BC_REPLY, > > BC_ACQUIRE_RESULT, > > + BC_FREE_BUFFER, BC_INCREFS, > > BC_ACQUIRE, > > + BC_RELEASE, BC_DECREFS, > > BC_INCREFS_DONE, > > + BC_ACQUIRE_DONE, > > BC_ATTEMPT_ACQUIRE, > > + BC_REGISTER_LOOPER, > > BC_ENTER_LOOPER, > > + BC_EXIT_LOOPER, > > BC_REQUEST_DEATH_NOTIFICATION, > > + BC_CLEAR_DEATH_NOTIFICATION, > > BC_DEAD_BINDER_DONE, > > + BR_ERROR, BR_OK, BR_TRANSACTION, > > BR_REPLY, > > + BR_ACQUIRE_RESULT, BR_DEAD_REPLY, > > + BR_TRANSACTION_COMPLETE, > > BR_INCREFS, BR_ACQUIRE, > > + BR_RELEASE, BR_DECREFS, > > BR_ATTEMPT_ACQUIRE, > > + BR_NOOP, BR_SPAWN_LOOPER, > > BR_FINISHED, > > + BR_DEAD_BINDER, > > BR_CLEAR_DEATH_NOTIFICATION_DONE, > > + BR_FAILED_REPLY]),,,[ #include > > <sys/types.h> > > Please list these constants sorted in one column like in other places. > Will do. > > +#include <linux/android/binder.h>])]) > > + > > I'm not quite sure what's expected from defined(__ANDROID__) in > binder.c and whether this check is consistent with conditions where > <linux/android/binder.h> is not available but defined(__ANDROID__) is > true. > > On android, the header is located at <linux/binder.h> and since it's a vital component in the system so it will be there. Regards, -- Antoine 'xdbob' Damhet LSE - System & Security ACDC/EPITA 2018
pgpyD0dTXNFh4.pgp
Description: Signature digitale OpenPGP
------------------------------------------------------------------------------ Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San Francisco, CA to explore cutting-edge tech and listen to tech luminaries present their vision of the future. This family event has something for everyone, including kids. Get more information and register today. http://sdm.link/attshape
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel