3 ms·

You don't really have to guess. The guy told us the AI didn't suggest this specific change: > The change to zero memory was my idea and my change. It was a rea

You don't really have to guess. The guy told us the AI didn't suggest this specific change: > The change to zero memory was my idea and my change. It was a reaction to a security report I got which caused use of an element past the end of an array. By zeroing the allocation I could ensure that misuse of that memory if a similar bug came up in the future could only cause a null ptr deref, which is better than the chance of a valid pointer. It got a claude co-authored tag on it as I got it to do some tidy ups of a series of commits, and that is just what it does when it makes any modification. It doesn't mean the change was written by claude. It was written by me. https://github.com/RsyncProject/rsync/issues/959#issuecomment-4625643726 https://github.com/RsyncProject/rsync/issues/959#issuecommen...

jagged-chisel 4mo ago
> … By zeroing the allocation … How does that prevent reading past the end of the buffer? Or change how bytes outside the buffer are used? Are these arrays of pointers so that the “null ptr deref” comment makes sense? Or am I the bozo and don’t know what’s happening here?
- kccqzy 4mo ago
  It doesn’t. It’s just that dereferencing a zeroed pointer reliably crashes the program (unless you specifically do funky things with mmap) but dereferencing garbage memory as a pointer could do a lot more insidious damage.
  - jagged-chisel 4mo ago
    My point is that the developer's comment doesn't make sense. Zeroing the allocated memory doesn't change anything about overrunning the buffer. edit: removed unnecessary examples
    - davrosthedalek 4mo ago
      Haven't looked at the code, but the allocated memory could be larger than necessary to make "off-by-one" or "off-by-a-few" errors less deadly. Then zeroing it out makes it even less so. Defense in depth. Or it's an allocation for an arena? The zeroing might help trigger 0 derefs earlier if the overrun happens for the object that are then allocated in the arena (and not by allocating more objects than the arena can provide)
    - Shish2k 4mo ago
      This doesn't prevent overrunning the buffer -- it means that when you do overrun the buffer, it does less damage
- deleted 4mo ago
  [deleted]
- wzdd 4mo ago
  The code is part of a function called expand item list. It looks like it over allocates memory and uses a bump pointer for internal allocation, only expanding the allocation when necessary. Thus OOB writes to the list would hit the allocated memory. You’re not a bozo but it is helpful to read the code.
GodelNumbering 4mo ago
okay I had not read this or any discussions there (except the one linked in the post), but this looks weirder. the comment you linked is a dev responding to what is very clearly a bot comment. I am sure they have good intentions and I have no reason to believe otherwise as I have no connection to the project whatsoever, but the original commit being 4-5 lines long (what did claude do then?) and the revert description is almost certainly written by an LLM makes in my mind the slop argument stronger. I hope if this doesn't come across as unkind towards the dev who gives their time and energy to the project. Grateful for that.
- wzdd 4mo ago
  It is certainly unkind, when a developer asserts the opposite of what you have assumed about their code, to double down and imply they are lying.
- scottlamb 4mo ago
  > the original commit being 4-5 lines long (what did claude do then?) I've said "rebase onto <newbase>" and let it handle all the merge conflicts. I wouldn't expect this particular commit to conflict with anything, but it could have been part of a big series where it'd be worth doing that instead of running the rebase command yourself. It wouldn't surprise me if I picked up some Co-Authored-By:s along the way.

jagged-chisel 4mo ago

> … By zeroing the allocation … How does that prevent reading past the end of the buffer? Or change how bytes outside the buffer are used? Are these arrays of pointers so that the “null ptr deref” comment makes sense? Or am I the bozo and don’t know what’s happening here?

kccqzy 4mo ago
It doesn’t. It’s just that dereferencing a zeroed pointer reliably crashes the program (unless you specifically do funky things with mmap) but dereferencing garbage memory as a pointer could do a lot more insidious damage.
- jagged-chisel 4mo ago
  My point is that the developer's comment doesn't make sense. Zeroing the allocated memory doesn't change anything about overrunning the buffer. edit: removed unnecessary examples
  - davrosthedalek 4mo ago
    Haven't looked at the code, but the allocated memory could be larger than necessary to make "off-by-one" or "off-by-a-few" errors less deadly. Then zeroing it out makes it even less so. Defense in depth. Or it's an allocation for an arena? The zeroing might help trigger 0 derefs earlier if the overrun happens for the object that are then allocated in the arena (and not by allocating more objects than the arena can provide)
  - Shish2k 4mo ago
    This doesn't prevent overrunning the buffer -- it means that when you do overrun the buffer, it does less damage
deleted 4mo ago
[deleted]
wzdd 4mo ago
The code is part of a function called expand item list. It looks like it over allocates memory and uses a bump pointer for internal allocation, only expanding the allocation when necessary. Thus OOB writes to the list would hit the allocated memory. You’re not a bozo but it is helpful to read the code.

GodelNumbering 4mo ago

okay I had not read this or any discussions there (except the one linked in the post), but this looks weirder. the comment you linked is a dev responding to what is very clearly a bot comment. I am sure they have good intentions and I have no reason to believe otherwise as I have no connection to the project whatsoever, but the original commit being 4-5 lines long (what did claude do then?) and the revert description is almost certainly written by an LLM makes in my mind the slop argument stronger. I hope if this doesn't come across as unkind towards the dev who gives their time and energy to the project. Grateful for that.

wzdd 4mo ago
It is certainly unkind, when a developer asserts the opposite of what you have assumed about their code, to double down and imply they are lying.
scottlamb 4mo ago
> the original commit being 4-5 lines long (what did claude do then?) I've said "rebase onto <newbase>" and let it handle all the merge conflicts. I wouldn't expect this particular commit to conflict with anything, but it could have been part of a big series where it'd be worth doing that instead of running the rebase command yourself. It wouldn't surprise me if I picked up some Co-Authored-By:s along the way.