Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Got it. I jumped to the wrong conclusion seeing sigaction() being used together with `rpmsigTbl`. I missed that that particular line was saving the old signal disposition, not setting the new one. That definitely makes more sense for the name "signal queue" :-). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-394629170___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Agreed, fab7348 removes the need for my commit. I notice exit() does not seem to be on the list of POSIX signal-safe functions (in contrast with `_exit()`). I guess in general, if rpm is really trying to clean up inside a signal handler, I'd expect a lot of really hairy edge cases. E.g. technically I think librpm might also be relying on `_free()` not being inlined (LTO). Otherwise, I think the compiler might have the opportunity to re-order freeing the db _before_ updating `*prev`. Leading to double-free if a signal hits at that point. https://github.com/rpm-software-management/rpm/blob/fab7348d9a338349e5727f87af734c0e20cfb7ad/lib/rpmdb.c#L421 Thank you for working on rpm. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-394375192___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
Eh. It can cause a bit of awkwardness or need for discussion when you get unlucky, agreed. So far I'm not very keen to send 3+ different PRs for small fixes to cleanup when they don't depend on each other. I appreciate the attention to detail. Cleanup fixes can be fiddly to get right even if there isn't a concern with the API. Perhaps one argument for a topic PR that gets you in the right mood for nitpicking :). What a fun commit "avoid double free ... assume that the object has been freed if it is not on the list." Good luck :). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359#issuecomment-368819363___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Fix error message for rpmlock (#355), by removing dead code (#358)
I guess I got a bit lost between them all. Thanks for the fix :). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/358#issuecomment-364911042___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Fix error message for rpmlock (#355), by removing dead code (#358)
Your commit shows the old message, but not the new one. What does it show now? If I'm decoding my past self's comments correctly, I was concerned not to have the message be EBADF since that also seems quite puzzling. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/358#issuecomment-364895839___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] rpmidxGetInternal(): leak on realloc() failure (#357)
I just counted from the top-level, using `git grep malloc | wc -l` (v.s. xmalloc). ~30 direct references to malloc() looks right for lib/. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/issues/357#issuecomment-347165957___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Fix error message for rpmlock (#355), by removing dead code (#358)
Thanks for review. I agree it's bad that we allow non-root users to block rpm :). Even if that had been fixed, I'd still prefer to rule out bogus (or unnecessarily confusing) error messages. They could occur in less common situations such as an LSM denying access to the root user. I can see that calling it all unreachable code was somewhat hyperbolic. I'm surprised if you could see/provoke any change in effect with the revised sequence though, other than the exact error message. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/358#issuecomment-346933360___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] 3 cleanup fixes (#359)
You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/359 -- Commit Summary -- * ndb glue: closeEnv() should always clear rdb->db_dbenv * rpmidxHandleObsolete(): Fix fd leak in error path * Fix leak with openDatabase() / rpmdbClose() * gitignore update -- File Changes -- M .gitignore (2) M lib/backend/ndb/glue.c (2) M lib/backend/ndb/rpmidx.c (6) M lib/rpmdb.c (17) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/359.patch https://github.com/rpm-software-management/rpm/pull/359.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/359 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Fix error message for rpmlock (#355), by removing dead code (#358)
(plus one bonus commit, hope that's ok) --- rpmlock_acquire() failed to set errno, when it failed to acquire a write-lock on a lock which was opened as read-only. errno was printed in the error message, so the error message was arbitrary. Typically the previous errno happened to be the right one, but if STDIN was not a TTY, then it was not. (Due to an intervening call to isatty()). A one-line fix to set errno appropriately would not fix #355. Because the appropriate errno for performing write operations on a read-only handle is EBADF. This is too generic to tell the user what the real problem is (i.e. that rpm was run without the necessary privilege to write to the lock file). This obstacle would disappear if we could avoid the automatic fallback from a read-write open() to a read-only open(). It turns out that rpmlock.h never exported a read-only lock feature, so this was never used. Remove the code for automatic fallback to read-only and read-only locks. This makes #355 go away. You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/358 -- Commit Summary -- * Fix error message for rpmlock (#355), by removing dead code * rpmlock_new(): use xcalloc() -- File Changes -- M lib/rpmlock.c (60) M lib/rpmlock.h (6) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/358.patch https://github.com/rpm-software-management/rpm/pull/358.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/358 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] rpmidxGetInternal(): leak on realloc() failure (#357)
When realloc() fails the original allocation remains, and should be freed at
some point.
Can rpmidxGetInternal() be converted to xrealloc() ? I'm unsure because there
might be a deliberate reason, e.g. why we still have 130 references directly to
malloc() and not xmalloc().
```
@@ -927,16 +927,14 @@ static int rpmidxGetInternal(rpmidxdb idxdb, const
unsigned char *key, unsigned
}
if (keyoff != x)
continue;
if ((nhits & 15) == 0) {
if (!hits) {
- hits = malloc(16 * sizeof(unsigned int));
+ hits = xmalloc(16 * sizeof(unsigned int));
} else {
- hits = realloc(hits, (nhits + 16) * sizeof(unsigned int));
+ hits = xrealloc(hits, (nhits + 16) * sizeof(unsigned int));
}
- if (!hits)
- return RPMRC_FAIL;
}
data = le2ha(ent + 4);
ovldata = (data & 0x8000) ? le2ha(idxdb->slot_mapped +
idxdb->nslots * 8 + 4 * h) : 0;
hits[nhits++] = decodedata(idxdb, data, ovldata, );
hits[nhits++] = datidx;
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/issues/357___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Only fallback to read-only locking for errno==EACCES (#356)
Ah, except there's no evidence the read-only locks were ever exposed outside rpmlock.c, let alone used. It'll be cleaner to remove the read-only locks to fix #355. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/356#issuecomment-343757276___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Only fallback to read-only locking for errno==EACCES (#356)
instead of for errno!=EROFS. File creation could be also denied due to ENOSPC, for example. Errors are generally easier to troubleshoot / debug if we abort early. It's not so great to throw away e.g. ENOSPC and continue in read-only mode. You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/356 -- Commit Summary -- * Only fallback to read-only locking for errno==EACCES -- File Changes -- M lib/rpmlock.c (2) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/356.patch https://github.com/rpm-software-management/rpm/pull/356.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/356 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] rpm without TTY gives wrong error message (#355)
Yeah, I just found it too. In principle the closest errno would be EBADF though, which is not much more user-friendly :(. So I'm not sure what the best response to this is. >EBADF cmd is F_SETLK or F_SETLKW and the file descriptor open mode > doesn't match with the type of lock requested. (same as `write()` on an fd which was not opened for writing). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/issues/355#issuecomment-343745434___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] rpm without TTY gives wrong error message (#355)
I don't understand. What are you asking for? This is a UI bug, triggered when a non-query `rpm` command is run without permissions _and_ without a tty on STDIN. I don't see what more information I can provide. Running `rpm` _with_ permissions doesn't print an error message; I checked and it doesn't matter whether I redirect STDIN or not. If you're saying this is issue closed as some flavour of WONTFIX, you forgot to close it. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/issues/355#issuecomment-343723528___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] rpm without TTY gives wrong error message (#355)
``` $ rpm -e mlocate error: can't create transaction lock on /var/lib/rpm/.rpm.lock (Permission denied) $ rpm -e mlocate https://github.com/rpm-software-management/rpm/issues/355___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
