On Thu, Mar 01, 2007 at 11:27:34AM -0800, Jean Tourrilhes wrote: > On Thu, Mar 01, 2007 at 08:42:09AM +0100, Jarek Poplawski wrote: > > On Wed, Feb 28, 2007 at 10:45:41AM -0800, Jean Tourrilhes wrote: > > > > > + > > > > > + if ((size <= 0) || (i >= num_envp)) > > > > > > > > Btw.: > > > > 1. if size == 10 and snprintf returns 9 (without NULL) > > > > then n == 10 (with NULL), so isn't it enough (here and above): > > > > > > > > if ((size < 0) || (i >= num_envp)) > > > > > > I just cut'n'pasted the code a few line above. If the original > > > code is incorrect, it need fixing. And it will need fixing in probably > > > a lot of places. > > > > I think you're kind of responsible for your part, at least. > > I personally don't go fixing stuff without having a full > undersunding of the context and why it was done this way. To me it > look this test was intentionally done this way, so there may be > something I don't know about. I guess I would need to double check > more closely what weid calculation the caller does with the buffer > size. > That's why I would prefer a separate patch about those issues > that give the opportunity for the stakeholder to really talk about > this, rather than slipping it under the carpet.
Sure, but adding a functionality is a kind of fixing, too. I don't blame you - I simply think that the patch like yours is an occasion for people reading this list to look at the adjacent code too. So I wrote about my doubts here hoping somebody with a better knowledge of this place could verify them. > In the worse case, this is not a bug, this just means that we > may not use the last char of the buffer. Sorry, I can't agree with this. > > > > > 2. shouldn't there be (here and above): > > > > > > > > envp[--i] = NULL; > > > > > > > > > > No, envp is local, so who cares. > > > > But envp[i] isn't (at least here). So, I guess, a caller > > of this function could care. > > Check the full source code of the caller, and you will see > that the caller does not care (and it's local to the caller). I can't agree with this, too. There is no reason to leave a bad pointer there - you need to analyze more than one caller to verify this now, and any changes in the future are endangered, too. > > > > > > + if ((size <= 0) || (i >= num_envp)) > > > > > + return -ENOMEM; > > > > And one more thing (not necessarily for you): > > ENOBUFS is probably more adequate here. > > This error should never happen. If it happens, the caller need > to be fixed. Yes, but an error handling should be correct or removed, if unnecessary. ... > Note that there are various other things that are puzzling in > this function. The internal object name and the symlinks are changed > even if the kcore rename returns an error. That does not seem right. > But, this is separate from this patch... So, I hope somebody will help to make this code perfect! Best regards, Jarek P. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
