Re: standardize and simplify GitHub submodule handling in ports?
On Sat, Aug 05, 2023 at 11:27:24PM +0200, Marc Espie wrote: > Some comments already. I haven't looked very closely. > On Sat, Aug 05, 2023 at 03:12:18PM -0400, Thomas Frohwein wrote: > > The current draft hijacks post-extract target, but it would be easy to > > add this to _post-extract-finalize in bsd.port.mk similar to how the > > post-extract commands from modules are handled, if this is of interest. > > Please do that. Thanks, I updated the draft. Realized that including it with MODULES=github is easiest and then this can just use MODGITHUB_post-extract and doesn't need any custom code in bsd.port.mk. I had a thinko in post-extract (needs to be '||', not '&&') which is also corrected. > > # where submodule distfiles will be stored > > GHSM_DIST_SUBDIR ?= gh-submodules > > Please keep to the GH_* subspace. > > Already, modules usually use MOD* variable names, but in the GH case, that > would be a bit long. I renamed GHSM_* to GH_*. I wouldn't mind using MODGH_* if that's an option, but MODGITHUB_* would be pretty unwieldy, especially if we were to make the existing GH_{ACCOUNT,PROJECT,TAGNAME} etc. part of this. [...] > Please do a single loop. That's slightly more readable for me. yes, done. [...] > Also please draft a diff for port-modules(5) I'm attaching a diff for port-modules.5, along with the updated github.port.mk. > I'm also wondering if keeping the main GH_* stuff in bsd.port.mk makes a lot > of sense, instead of grouping everything in github.port.mk I'm for it, maybe as a second step after the module for just the submodule handling is done because there would be a lot of ports churn with moving the main GH_* stuff out of bsd.port.mk. # List of static dependencies. The format is: # account project tag_or_commit target_dir # license # Example: # GH_SUBMODULES += moonlight-stream moonlight-common-c \ # c9426a6a71c4162e65dde8c0c71a25f1dbca46ba \ # third-party/moonlight-common-c # GPL-v3.0+ GH_SUBMODULES ?= # Master site for github tarballs GH_MASTER_SITES ?= https://github.com/ # where submodule distfiles will be stored GH_DIST_SUBDIR ?= gh-submodules # where submodules will be extracted to GH_WRKSRC ?=${WRKSRC} # Grab submodules by default with MASTER_SITES8. (Don't use 9 to avoid collision # with language-specific mechanisms, like devel/cargo or lang/go.) GH_MASTER_SITESN ?= 8 MASTER_SITES${GH_MASTER_SITESN} ?= ${GH_MASTER_SITES} # Default GitHub distfile suffix GH_SUFX ?= .tar.gz .if defined(DISTNAME) DISTFILES ?= ${DISTNAME}${EXTRACT_SUFX} .elif !empty(GH_ACCOUNT) && !empty(GH_PROJECT) DISTFILES ?= ${GH_DISTFILE} .endif # post-extract target for moving the submodules to GH_WRKSRC MODGITHUB_post-extract = \ @${ECHO_MSG} "moving GitHub submodules to ${GH_WRKSRC}" ; \ mkdir -p ${GH_WRKSRC} ; .for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES} DISTFILES += ${GH_DIST_SUBDIR}/{}${_ghaccount}/${_ghproject}/archive/${_ghtagcommit}${GH_SUFX}:${GH_MASTER_SITESN} MODGITHUB_post-extract += \ test -d ${GH_WRKSRC}/${_targetdir} && \ rm -rf ${GH_WRKSRC}/${_targetdir} ; \ mv ${WRKDIR}/${_ghproject}-${_ghtagcommit} ${GH_WRKSRC}/${_targetdir} ; .endfor Index: port-modules.5 === RCS file: /cvs/src/share/man/man5/port-modules.5,v retrieving revision 1.264 diff -u -p -r1.264 port-modules.5 --- port-modules.5 9 May 2023 19:44:06 - 1.264 +++ port-modules.5 6 Aug 2023 01:48:22 - @@ -744,6 +744,15 @@ contains c++, this module provides .Ev MODGCC4_CPPLIBDEP and .Ev MODGCC4_CPPWANTLIB . +.It github +Set +.Li GH_SUBMODULES += w x y z +for each GitHub submodule to be used in the port, specifying GitHub account, project, commit hash, and the target directory relative to +.Ev GH_WRKSRC +.Po +defaults to +.Ev WRKSRC +.Pc . .It gnu This module is documented in the main .Xr bsd.port.mk 5
hardclock(9), roundrobin: make roundrobin() an independent clock interrupt
This is the next piece of the clock interrupt reorganization patch series. This patch removes the roundrobin() call from hardclock() and makes roundrobin() an independent clock interrupt. - Revise roundrobin() to make it a valid clock interrupt callback. It remains periodic. It still runs at one tenth of the hardclock frequency. - Account for multiple expirations in roundrobin(). If two or more intervals have elapsed we set SPCF_SHOULDYIELD immediately. This preserves existing behavior: hardclock() is called multiple times during clockintr_hardclock() if clock interrupts are blocked for long enough. - Each schedstate_percpu has its own roundrobin() handle, spc_roundrobin. spc_roundrobin is established during sched_init_cpu(), staggered during the first clockintr_cpu_init() call, and advanced during clockintr_cpu_init(). Expirations during suspend/resume are discarded. - spc_rrticks and rrticks_init are now useless. Delete them. ok? Also, yes, I see the growing pile of scheduler-controlled clock interrupt handles. My current plan is to move the setup code at the end of clockintr_cpu_init() to a different routine, maybe something like "sched_start_cpu()". On the primary CPU you'd call it immediately after cpu_initclocks(). On secondary CPUs you'd call it at the end of cpu_hatch() just before cpu_switchto(). In any case, we will need to find a home for that code someplace. It can't stay in clockintr_cpu_init() forever. Index: kern/sched_bsd.c === RCS file: /cvs/src/sys/kern/sched_bsd.c,v retrieving revision 1.79 diff -u -p -r1.79 sched_bsd.c --- kern/sched_bsd.c5 Aug 2023 20:07:55 - 1.79 +++ kern/sched_bsd.c5 Aug 2023 22:15:25 - @@ -56,7 +56,6 @@ intlbolt; /* once a second sleep address */ -intrrticks_init; /* # of hardclock ticks per roundrobin() */ #ifdef MULTIPROCESSOR struct __mp_lock sched_lock; @@ -69,21 +68,23 @@ uint32_tdecay_aftersleep(uint32_t, uin * Force switch among equal priority processes every 100ms. */ void -roundrobin(struct cpu_info *ci) +roundrobin(struct clockintr *cl, void *cf) { + struct cpu_info *ci = curcpu(); struct schedstate_percpu *spc = &ci->ci_schedstate; + uint64_t count; - spc->spc_rrticks = rrticks_init; + count = clockintr_advance(cl, hardclock_period * 10); if (ci->ci_curproc != NULL) { - if (spc->spc_schedflags & SPCF_SEENRR) { + if (spc->spc_schedflags & SPCF_SEENRR || count >= 2) { /* * The process has already been through a roundrobin * without switching and may be hogging the CPU. * Indicate that the process should yield. */ atomic_setbits_int(&spc->spc_schedflags, - SPCF_SHOULDYIELD); + SPCF_SEENRR | SPCF_SHOULDYIELD); } else { atomic_setbits_int(&spc->spc_schedflags, SPCF_SEENRR); @@ -695,8 +696,6 @@ scheduler_start(void) * its job. */ timeout_set(&schedcpu_to, schedcpu, &schedcpu_to); - - rrticks_init = hz / 10; schedcpu(&schedcpu_to); #ifndef SMALL_KERNEL Index: kern/kern_sched.c === RCS file: /cvs/src/sys/kern/kern_sched.c,v retrieving revision 1.84 diff -u -p -r1.84 kern_sched.c --- kern/kern_sched.c 5 Aug 2023 20:07:55 - 1.84 +++ kern/kern_sched.c 5 Aug 2023 22:15:25 - @@ -102,6 +102,12 @@ sched_init_cpu(struct cpu_info *ci) if (spc->spc_profclock == NULL) panic("%s: clockintr_establish profclock", __func__); } + if (spc->spc_roundrobin == NULL) { + spc->spc_roundrobin = clockintr_establish(&ci->ci_queue, + roundrobin); + if (spc->spc_roundrobin == NULL) + panic("%s: clockintr_establish roundrobin", __func__); + } kthread_create_deferred(sched_kthreads_create, ci); Index: kern/kern_clockintr.c === RCS file: /cvs/src/sys/kern/kern_clockintr.c,v retrieving revision 1.30 diff -u -p -r1.30 kern_clockintr.c --- kern/kern_clockintr.c 5 Aug 2023 20:07:55 - 1.30 +++ kern/kern_clockintr.c 5 Aug 2023 22:15:25 - @@ -204,6 +204,11 @@ clockintr_cpu_init(const struct intrcloc clockintr_stagger(spc->spc_profclock, profclock_period, multiplier, MAXCPUS); } + if (spc->spc_roundrobin->cl_expiration == 0) { + clockintr_stagger(spc->spc_roundrobin, hardclock_period, + multiplier, MAXCPUS); + } + clockintr_advance(spc->spc_roundr
Re: standardize and simplify GitHub submodule handling in ports?
Some comments already. I haven't looked very closely. On Sat, Aug 05, 2023 at 03:12:18PM -0400, Thomas Frohwein wrote: > The current draft hijacks post-extract target, but it would be easy to > add this to _post-extract-finalize in bsd.port.mk similar to how the > post-extract commands from modules are handled, if this is of interest. Please do that. > # where submodule distfiles will be stored > GHSM_DIST_SUBDIR ?= gh-submodules Please keep to the GH_* subspace. Already, modules usually use MOD* variable names, but in the GH case, that would be a bit long. > .for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES} > DISTFILES += > ${GHSM_DIST_SUBDIR}/{}${_ghaccount}/${_ghproject}/archive/${_ghtagcommit}${GH_SUFX}:${GHSM_MASTER_SITESN} > .endfor > > # post-extract target for moving the submodules to the target directories > GHSM_post-extract = > .for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES} > GHSM_post-extract += \ > test -d ${GHSM_WRKSR}/${_targetdir} || rm -rf > ${GHSM_WRKSRC}/${_targetdir} ; \ That line is weird. > mv ${WRKDIR}/${_ghproject}-${_ghtagcommit} ${GHSM_WRKSRC}/${_targetdir} > ; > .endfor Please do a single loop. That's slightly more readable for me. > # XXX: would best belong in _post-extract-finalize in bsd.port.mk rather than > # hijacking post-extract here > post-extract: > @${ECHO_MSG} "moving GitHub submodules to ${GHSM_WRKSRC}" ; > mkdir -p ${GHSM_WRKSRC} ; > ${GHSM_post-extract} Also please draft a diff for port-modules(5) I'm also wondering if keeping the main GH_* stuff in bsd.port.mk makes a lot of sense, instead of grouping everything in github.port.mk
standardize and simplify GitHub submodule handling in ports?
Hi, GitHub projects using submodules seems to be a common enough case that I'm wondering if it would be helpful to simplify and standardize it. I got the idea when I saw how the modules for go and cargo pull in their modules or crates, respectively. The current state is that projects with submodules are either manually packaged and hosted, or a combination of of MASTER_SITES0 through 9, manual DISTFILES setting, and post-extract movement are used. What both have in common is at least some maintenance burden and a significant barrier for especially for those newer to the ports system. The diff below proposes a module github.port.mk that aims to simplify this process. At its core, the main way to manage submodules is to add a line like the following: GH_SUBMODULES+= luvit luv 093a977b82077591baefe1e880d37dfa2730bd54 \ luv # Apache-2.0 This way the GitHub tarball from the commit 093a977b from the project luv by account luvit is added to the distfiles, and then the extracted files are moved to ${GHSM_WRKSRC}/luv (GHSM_WRKSRC defaults to ${WRKSRC}). I saw license comments used with MODCARGO_CRATES and think this would also be a good location for the submodule licenses. This way, the multi-step setup and maintenance in MASTER_SITESX, DISTFILES, and post-extract is reduced to essentially one location. The current draft hijacks post-extract target, but it would be easy to add this to _post-extract-finalize in bsd.port.mk similar to how the post-extract commands from modules are handled, if this is of interest. I'm attaching the github.port.mk file, as well as 3 diffs to show how the simplified Makefiles look with this. A quick grep through the ports tree shows there are at least a couple of dozen ports that could benefit from a rework... # List of static dependencies. The format is: # account project tag_or_commit target_dir # license # Example: # GH_SUBMODULES += moonlight-stream moonlight-common-c \ # c9426a6a71c4162e65dde8c0c71a25f1dbca46ba \ # third-party/moonlight-common-c # GPL-v3.0+ GH_SUBMODULES ?= # Master site for github tarballs GH_MASTER_SITES ?= https://github.com/ # where submodule distfiles will be stored GHSM_DIST_SUBDIR ?= gh-submodules # where submodules will be extracted to GHSM_WRKSRC ?= ${WRKSRC} # Grab submodules by default with MASTER_SITES8. (Don't use 9 to avoid collision # with language-specific mechanisms, like devel/cargo or lang/go.) GHSM_MASTER_SITESN ?= 8 MASTER_SITES${GHSM_MASTER_SITESN} ?=${GH_MASTER_SITES} # Default GitHub distfile suffix GH_SUFX ?= .tar.gz .if defined(DISTNAME) DISTFILES ?= ${DISTNAME}${EXTRACT_SUFX} .elif !empty(GH_ACCOUNT) && !empty(GH_PROJECT) DISTFILES ?= ${GH_DISTFILE} .endif .for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES} DISTFILES += ${GHSM_DIST_SUBDIR}/{}${_ghaccount}/${_ghproject}/archive/${_ghtagcommit}${GH_SUFX}:${GHSM_MASTER_SITESN} .endfor # post-extract target for moving the submodules to the target directories GHSM_post-extract = .for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES} GHSM_post-extract += \ test -d ${GHSM_WRKSR}/${_targetdir} || rm -rf ${GHSM_WRKSRC}/${_targetdir} ; \ mv ${WRKDIR}/${_ghproject}-${_ghtagcommit} ${GHSM_WRKSRC}/${_targetdir} ; .endfor # XXX: would best belong in _post-extract-finalize in bsd.port.mk rather than # hijacking post-extract here post-extract: @${ECHO_MSG} "moving GitHub submodules to ${GHSM_WRKSRC}" ; mkdir -p ${GHSM_WRKSRC} ; ${GHSM_post-extract} Index: Makefile === RCS file: /cvs/ports/editors/neovim/Makefile,v retrieving revision 1.37 diff -u -p -r1.37 Makefile --- Makefile14 Jun 2023 07:47:57 - 1.37 +++ Makefile5 Aug 2023 19:06:09 - @@ -23,22 +23,20 @@ CATEGORIES =editors devel HOMEPAGE = https://neovim.io MAINTAINER = Edd Barrett +# Move static deps source code under WRKDIST so that they can be patched. +STATIC_DEPS_WRKSRC = ${WRKDIST}/static-deps +GHSM_WRKSRC = ${STATIC_DEPS_WRKSRC} + # The versions listed here must match those in cmake.deps/CMakeLists.txt. -LUV_VER = 093a977b82077591baefe1e880d37dfa2730bd54 -LUAJIT_VER = 505e2c03de35e2718eef0d2d3660712e06dadf1f -LUACOMPAT_VER =v0.9 - -MASTER_SITES0 =https://github.com/luvit/luv/archive/ -MASTER_SITES1 = https://github.com/LuaJIT/LuaJIT/archive/ -MASTER_SITES2 = https://github.com/keplerproject/lua-compat-5.3/archive/ -DISTFILES =${DISTNAME}${EXTRACT_SUFX} \ - luv-{}${LUV_VER}${EXTRACT_SUFX}:0 \ - luajit-{}${LUAJIT_VER}${EXTRACT_SUFX}:1 \ - lua-compat-5.3-{}${LUACOMPAT_VER}${EXTRACT_SUFX}:2 - -# Neovim: Apache 2.0 + Vim License -# LuaJIT: MIT + public domain -# libluv: Apache 2.0 +GH_SUBMODULES+=luvit luv 093a977b82077591baefe1e880d37dfa2730bd54 \ + luv # Apa
Re: [www] fix broken links on loongson.html
Hi, I noticed this today and commited your change. mbuhl On Tue, Jun 02, 2020 at 12:33:36PM +, Yifei Zhan wrote: > Hi, > > Several links on loongson.html are now broken as zkml.lemote.com is no > longer being resolved. > > I think it's a good idea to redirect them to their archived version on > archive.org. > > Cheers, > Yifei Zhan > > > Index: loongson.html > === > RCS file: /cvs/www/loongson.html,v > retrieving revision 1.66 > diff -u -p -r1.66 loongson.html > --- loongson.html 19 May 2020 02:02:10 - 1.66 > +++ loongson.html 31 May 2020 06:55:56 - > @@ -73,16 +73,16 @@ Specific hardware support is then writte > At the moment, the following machines are supported: > > > - href="http://zkml.lemote.com/en/products/mini-computer/2010/0310/111.html";>Lemote > Fuloong 2F > + href="https://web.archive.org/web/20190101154956/http://zkml.lemote.com/en/products/mini-computer/2010/0310/111.html";>Lemote > Fuloong 2F > mini-PC > > All on-board devices are supported, but the framebuffer is currently limited > to the 640x400x8 video mode set up by the firmware. > - href="http://zkml.lemote.com/en/products/all-in-one/2010/0311/122.html";>Lemote > Lynloong all-in-one-PC > + href="https://web.archive.org/web/20190101150011/http://zkml.lemote.com/en/products/all-in-one/2010/0311/122.html";>Lemote > Lynloong all-in-one-PC > > All on-board devices are supported, but the framebuffer is currently limited > to the 1360x768x16 video mode set up by the firmware. > - href="http://zkml.lemote.com/en/products/Notebook/2010/0310/112.html";>Lemote > Yeeloong > + href="https://web.archive.org/web/20160703160118/http://zkml.lemote.com/en/products/Notebook/2010/0310/112.html";>Lemote > Yeeloong > netbook > > Both the 8.9" and 10.1" models are supported.
Re: ldd: check read return value to avoid unitialized struct fields
That looks better. Lucas wrote: > Theo Buehler wrote: > > > - if (pread(fd, phdr, size, ehdr.e_phoff) != size) { > > > + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) { > > > > did you intend to check for == -1? > > > > > warn("read(%s)", name); > > > > should that not say pread? > > Indeed, thanks for spotting both things. > > > --- > commit 92f58b2a1cd576c3e72303004388ab1e9709e327 (ldd-read-rv) > from: Lucas > date: Sat Aug 5 16:34:16 2023 UTC > > Check {,p}read return values consistently > > Check that read performs a full header read. Explicitly check against -1 > for failure instead of < 0. Split pread error message between error > handling and short reads. Promote size from int to size_t. > > M libexec/ld.so/ldd/ldd.c > > diff 194ff02fb6be247e27fe964e901d891d99ec0b74 > 92f58b2a1cd576c3e72303004388ab1e9709e327 > commit - 194ff02fb6be247e27fe964e901d891d99ec0b74 > commit + 92f58b2a1cd576c3e72303004388ab1e9709e327 > blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365 > blob + 532feb9855a03480c289fa2188768657a4f82e7c > --- libexec/ld.so/ldd/ldd.c > +++ libexec/ld.so/ldd/ldd.c > @@ -96,7 +96,9 @@ doit(char *name) > { > Elf_Ehdr ehdr; > Elf_Phdr *phdr; > - int fd, i, size, status, interp=0; > + size_t size; > + ssize_t nr; > + int fd, i, status, interp=0; > char buf[PATH_MAX]; > struct stat st; > void * dlhandle; > @@ -118,11 +120,16 @@ doit(char *name) > return 1; > } > > - if (read(fd, &ehdr, sizeof(ehdr)) < 0) { > + if ((nr = read(fd, &ehdr, sizeof(ehdr))) == -1) { > warn("read(%s)", name); > close(fd); > return 1; > } > + if (nr != sizeof(ehdr)) { > + warnx("%s: incomplete ELF header", name); > + close(fd); > + return 1; > + } > > if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) { > warnx("%s: not an ELF executable", name); > @@ -140,12 +147,18 @@ doit(char *name) > err(1, "reallocarray"); > size = ehdr.e_phnum * sizeof(Elf_Phdr); > > - if (pread(fd, phdr, size, ehdr.e_phoff) != size) { > - warn("read(%s)", name); > + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) == -1) { > + warn("pread(%s)", name); > close(fd); > free(phdr); > return 1; > } > + if (nr != size) { > + warnx("%s: incomplete program header", name); > + close(fd); > + free(phdr); > + return 1; > + } > close(fd); > > for (i = 0; i < ehdr.e_phnum; i++) >
Re: ldd: check read return value to avoid unitialized struct fields
Theo Buehler wrote: > > - if (pread(fd, phdr, size, ehdr.e_phoff) != size) { > > + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) { > > did you intend to check for == -1? > > > warn("read(%s)", name); > > should that not say pread? Indeed, thanks for spotting both things. --- commit 92f58b2a1cd576c3e72303004388ab1e9709e327 (ldd-read-rv) from: Lucas date: Sat Aug 5 16:34:16 2023 UTC Check {,p}read return values consistently Check that read performs a full header read. Explicitly check against -1 for failure instead of < 0. Split pread error message between error handling and short reads. Promote size from int to size_t. M libexec/ld.so/ldd/ldd.c diff 194ff02fb6be247e27fe964e901d891d99ec0b74 92f58b2a1cd576c3e72303004388ab1e9709e327 commit - 194ff02fb6be247e27fe964e901d891d99ec0b74 commit + 92f58b2a1cd576c3e72303004388ab1e9709e327 blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365 blob + 532feb9855a03480c289fa2188768657a4f82e7c --- libexec/ld.so/ldd/ldd.c +++ libexec/ld.so/ldd/ldd.c @@ -96,7 +96,9 @@ doit(char *name) { Elf_Ehdr ehdr; Elf_Phdr *phdr; - int fd, i, size, status, interp=0; + size_t size; + ssize_t nr; + int fd, i, status, interp=0; char buf[PATH_MAX]; struct stat st; void * dlhandle; @@ -118,11 +120,16 @@ doit(char *name) return 1; } - if (read(fd, &ehdr, sizeof(ehdr)) < 0) { + if ((nr = read(fd, &ehdr, sizeof(ehdr))) == -1) { warn("read(%s)", name); close(fd); return 1; } + if (nr != sizeof(ehdr)) { + warnx("%s: incomplete ELF header", name); + close(fd); + return 1; + } if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) { warnx("%s: not an ELF executable", name); @@ -140,12 +147,18 @@ doit(char *name) err(1, "reallocarray"); size = ehdr.e_phnum * sizeof(Elf_Phdr); - if (pread(fd, phdr, size, ehdr.e_phoff) != size) { - warn("read(%s)", name); + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) == -1) { + warn("pread(%s)", name); close(fd); free(phdr); return 1; } + if (nr != size) { + warnx("%s: incomplete program header", name); + close(fd); + free(phdr); + return 1; + } close(fd); for (i = 0; i < ehdr.e_phnum; i++)
Re: ldd: check read return value to avoid unitialized struct fields
> - if (pread(fd, phdr, size, ehdr.e_phoff) != size) { > + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) { did you intend to check for == -1? > warn("read(%s)", name); should that not say pread?
Re: ldd: check read return value to avoid unitialized struct fields
"Theo de Raadt" wrote: > Nope, that is not correct. > > errno is not being cleared. It just happens to be zero. Future > code changes could insert another operation above which would set > errno, and then this would print a report about that error. Although I was being sarcastic with """Everything is alright""", yes, correct. Point taken. > No, your diff is still wrong. > > errno is only updated when a system call returns -1. > > So your diff is looking at an old, unrelated, errno. How? This is now correctly looking at errno only when {,p}read returns -1, and is using warnx in the other cases.
Re: ldd: check read return value to avoid unitialized struct fields
Lucas wrote: > "Theo de Raadt" wrote: > > What errno is being printed here? > > """Everything is alright""" error, > > $ : >empty && ./obj/ldd empty > ldd: read(empty): Undefined error: 0 > > which would be the same as a short read in the pread below. Nope, that is not correct. errno is not being cleared. It just happens to be zero. Future code changes could insert another operation above which would set errno, and then this would print a report about that error. No, your diff is still wrong. errno is only updated when a system call returns -1. So your diff is looking at an old, unrelated, errno. And instead of the previous diff which did this once, you are now doing it 5 times. > --- > commit f1255c0035aa37752a298b752fd20215a1d7adef (ldd-read-rv) > from: Lucas > date: Sat Aug 5 14:36:58 2023 UTC > > Check {,p}read return values consistently > > Check that read performs a full header read. Explicitly check against -1 > for failure instead of < 0. Split pread error message between error > handling and short reads. Promote size from int to size_t. > > M libexec/ld.so/ldd/ldd.c > > diff 7b0c383483702d9a26856c2b4754abb44950ed82 > f1255c0035aa37752a298b752fd20215a1d7adef > commit - 7b0c383483702d9a26856c2b4754abb44950ed82 > commit + f1255c0035aa37752a298b752fd20215a1d7adef > blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365 > blob + 12777f2420a6a74f9f456f080c207bf47760b258 > --- libexec/ld.so/ldd/ldd.c > +++ libexec/ld.so/ldd/ldd.c > @@ -96,7 +96,9 @@ doit(char *name) > { > Elf_Ehdr ehdr; > Elf_Phdr *phdr; > - int fd, i, size, status, interp=0; > + size_t size; > + ssize_t nr; > + int fd, i, status, interp=0; > char buf[PATH_MAX]; > struct stat st; > void * dlhandle; > @@ -118,11 +120,16 @@ doit(char *name) > return 1; > } > > - if (read(fd, &ehdr, sizeof(ehdr)) < 0) { > + if ((nr = read(fd, &ehdr, sizeof(ehdr))) == -1) { > warn("read(%s)", name); > close(fd); > return 1; > } > + if (nr != sizeof(ehdr)) { > + warnx("%s: incomplete ELF header", name); > + close(fd); > + return 1; > + } > > if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) { > warnx("%s: not an ELF executable", name); > @@ -140,12 +147,18 @@ doit(char *name) > err(1, "reallocarray"); > size = ehdr.e_phnum * sizeof(Elf_Phdr); > > - if (pread(fd, phdr, size, ehdr.e_phoff) != size) { > + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) { > warn("read(%s)", name); > close(fd); > free(phdr); > return 1; > } > + if (nr != size) { > + warnx("%s: incomplete program header", name); > + close(fd); > + free(phdr); > + return 1; > + } > close(fd); > > for (i = 0; i < ehdr.e_phnum; i++) >
Re: ldd: check read return value to avoid unitialized struct fields
"Theo de Raadt" wrote: > What errno is being printed here? """Everything is alright""" error, $ : >empty && ./obj/ldd empty ldd: read(empty): Undefined error: 0 which would be the same as a short read in the pread below. Bigger suggestion below, addressing both read and pread. Also promoted size to a size_t, as the multiplication could overflow an int. read < 0 was changed into read == -1 for alignment with read(2). There are an open and wait that could receive the same < 0 vs == -1 treatment. Bike can be repainted at will. -Lucas --- commit f1255c0035aa37752a298b752fd20215a1d7adef (ldd-read-rv) from: Lucas date: Sat Aug 5 14:36:58 2023 UTC Check {,p}read return values consistently Check that read performs a full header read. Explicitly check against -1 for failure instead of < 0. Split pread error message between error handling and short reads. Promote size from int to size_t. M libexec/ld.so/ldd/ldd.c diff 7b0c383483702d9a26856c2b4754abb44950ed82 f1255c0035aa37752a298b752fd20215a1d7adef commit - 7b0c383483702d9a26856c2b4754abb44950ed82 commit + f1255c0035aa37752a298b752fd20215a1d7adef blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365 blob + 12777f2420a6a74f9f456f080c207bf47760b258 --- libexec/ld.so/ldd/ldd.c +++ libexec/ld.so/ldd/ldd.c @@ -96,7 +96,9 @@ doit(char *name) { Elf_Ehdr ehdr; Elf_Phdr *phdr; - int fd, i, size, status, interp=0; + size_t size; + ssize_t nr; + int fd, i, status, interp=0; char buf[PATH_MAX]; struct stat st; void * dlhandle; @@ -118,11 +120,16 @@ doit(char *name) return 1; } - if (read(fd, &ehdr, sizeof(ehdr)) < 0) { + if ((nr = read(fd, &ehdr, sizeof(ehdr))) == -1) { warn("read(%s)", name); close(fd); return 1; } + if (nr != sizeof(ehdr)) { + warnx("%s: incomplete ELF header", name); + close(fd); + return 1; + } if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) { warnx("%s: not an ELF executable", name); @@ -140,12 +147,18 @@ doit(char *name) err(1, "reallocarray"); size = ehdr.e_phnum * sizeof(Elf_Phdr); - if (pread(fd, phdr, size, ehdr.e_phoff) != size) { + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) { warn("read(%s)", name); close(fd); free(phdr); return 1; } + if (nr != size) { + warnx("%s: incomplete program header", name); + close(fd); + free(phdr); + return 1; + } close(fd); for (i = 0; i < ehdr.e_phnum; i++)